| Message ID | 20190228185128.43982-1-gert@greenie.muc.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Copy one byte less in strncpynt() | expand |
Hi, On 28/02/2019 19:51, Gert Doering wrote: > While the existing code is not wrong and will never cause an overflow, > it will copy (on a too-long source string) "maxlen" bytes to dest, and > then overwrite the last byte just copied with "0" - which causes a > warning in gcc 9 about filling the target buffer "up to the end, > with no room for a trailing 0 anymore". > > Reducing the maximum bytes-to-be-copied to "maxlen -1", because the > last byte will be stamped with 0 anyway. > > Signed-off-by: Gert Doering <gert@greenie.muc.de> > --- > src/openvpn/buffer.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h > index a4fe6f9b..52de5a2b 100644 > --- a/src/openvpn/buffer.h > +++ b/src/openvpn/buffer.h > @@ -347,7 +347,8 @@ buf_set_read(struct buffer *buf, const uint8_t *data, int size) > static inline void > strncpynt(char *dest, const char *src, size_t maxlen) > { > - strncpy(dest, src, maxlen); > + ASSERT(maxlen>0); > + strncpy(dest, src, maxlen-1); > if (maxlen > 0) This if condition makes me think that this function is allowed to be invoked with maxlen == 0. However you are now introducing an ASSERT() which would stop the execution in that case. Either the ASSERT() is right, and then the if condition should be removed, or the ASSERT() is wrong and should not be introduced. > { > dest[maxlen - 1] = 0; > Other than that the change makes sense. Regards,
diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h index a4fe6f9b..52de5a2b 100644 --- a/src/openvpn/buffer.h +++ b/src/openvpn/buffer.h @@ -347,7 +347,8 @@ buf_set_read(struct buffer *buf, const uint8_t *data, int size) static inline void strncpynt(char *dest, const char *src, size_t maxlen) { - strncpy(dest, src, maxlen); + ASSERT(maxlen>0); + strncpy(dest, src, maxlen-1); if (maxlen > 0) { dest[maxlen - 1] = 0;
While the existing code is not wrong and will never cause an overflow, it will copy (on a too-long source string) "maxlen" bytes to dest, and then overwrite the last byte just copied with "0" - which causes a warning in gcc 9 about filling the target buffer "up to the end, with no room for a trailing 0 anymore". Reducing the maximum bytes-to-be-copied to "maxlen -1", because the last byte will be stamped with 0 anyway. Signed-off-by: Gert Doering <gert@greenie.muc.de> --- src/openvpn/buffer.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)