| Message ID | 20230328151233.217296-1-frank@lichtenheld.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel] buffer: use memcpy in buf_catrunc | expand |
Hi, On 28/03/2023 17:12, Frank Lichtenheld wrote: > Since we use strlen() to determine the length > and then check it ourselves, there is really > no point in using strncpy. > > But the compiler might complain that we use > the output of strlen() for the length of > strncpy which is usually a sign for bugs: > > error: ‘strncpy’ specified bound depends > on the length of the source argument > [-Werror=stringop-overflow=] > > Warning was at least triggered for > mingw-gcc version 10-win32 20220113. > > Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com> > --- > src/openvpn/buffer.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c > index d099795b..de40d690 100644 > --- a/src/openvpn/buffer.c > +++ b/src/openvpn/buffer.c > @@ -317,9 +317,9 @@ buf_catrunc(struct buffer *buf, const char *str) > if (buf_forward_capacity(buf) <= 1) > { > int len = (int) strlen(str) + 1; > - if (len < buf_forward_capacity_total(buf)) > + if ((len > 0) && (len < buf_forward_capacity_total(buf))) len cannot be negative, but I guess you want to check that it is actually not 0. Just a style thing I guess? (Because memcpy can happily deal with len=0.) > { > - strncpynt((char *)(buf->data + buf->capacity - len), str, len); > + memcpy((void *)(buf->data + buf->capacity - len), (void *)str, (size_t)len); I'd throw away the casts to 'void *'. They are not really needed as you can assign everything to 'void *'. Cheers, > } > } > }
Am 28.03.23 um 17:12 schrieb Frank Lichtenheld: > Since we use strlen() to determine the length > and then check it ourselves, there is really > no point in using strncpy. > > But the compiler might complain that we use > the output of strlen() for the length of > strncpy which is usually a sign for bugs: > > error: ‘strncpy’ specified bound depends > on the length of the source argument > [-Werror=stringop-overflow=] > > Warning was at least triggered for > mingw-gcc version 10-win32 20220113. > > Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com> > --- > src/openvpn/buffer.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c > index d099795b..de40d690 100644 > --- a/src/openvpn/buffer.c > +++ b/src/openvpn/buffer.c > @@ -317,9 +317,9 @@ buf_catrunc(struct buffer *buf, const char *str) > if (buf_forward_capacity(buf) <= 1) > { > int len = (int) strlen(str) + 1; > - if (len < buf_forward_capacity_total(buf)) > + if ((len > 0) && (len < buf_forward_capacity_total(buf))) I hope you don't mind but there are a few things I would like to bring to attention. 1. The first is clamping and wrapping around: strlen(someptr) returns size_t, and this can be >= INT_MAX. Add +1 and this wraps to INT_MIN. While you now trap this with the new "len > 0", there does not seem to be any error handling, in that the function silently skips its actual function. Is this what such a function should do? Or should it be "complain an give up"? > { > - strncpynt((char *)(buf->data + buf->capacity - len), str, len); > + memcpy((void *)(buf->data + buf->capacity - len), (void *)str, (size_t)len); 2. This opens the door for another compiler warning, which is that you are casting away the const-ness of "str". It is const char * when passed in, but you cast it to void * (non-const). The 2nd argument of memcpy would need to be cast to (const void *) to avoid that. memcpy is declared (..., const void *restrict src, ...), so that's safe. 3. However, strncpynt would set buf->data[buf->capacity] = '\0', which is now missing. So: NAK, please revise on #3. Cheers, Matthias
Hi, On 28/03/2023 20:51, Matthias Andree wrote: > Am 28.03.23 um 17:12 schrieb Frank Lichtenheld: >> Since we use strlen() to determine the length >> and then check it ourselves, there is really >> no point in using strncpy. >> >> But the compiler might complain that we use >> the output of strlen() for the length of >> strncpy which is usually a sign for bugs: >> >> error: ‘strncpy’ specified bound depends >> on the length of the source argument >> [-Werror=stringop-overflow=] >> >> Warning was at least triggered for >> mingw-gcc version 10-win32 20220113. >> >> Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com> >> --- >> src/openvpn/buffer.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c >> index d099795b..de40d690 100644 >> --- a/src/openvpn/buffer.c >> +++ b/src/openvpn/buffer.c >> @@ -317,9 +317,9 @@ buf_catrunc(struct buffer *buf, const char *str) >> if (buf_forward_capacity(buf) <= 1) >> { >> int len = (int) strlen(str) + 1; >> - if (len < buf_forward_capacity_total(buf)) >> + if ((len > 0) && (len < buf_forward_capacity_total(buf))) > > I hope you don't mind but there are a few things I would like to bring > to attention. > > 1. The first is clamping and wrapping around: > > strlen(someptr) returns size_t, and this can be >= INT_MAX. Add +1 and > this wraps to INT_MIN. > While you now trap this with the new "len > 0", there does not seem to > be any error handling, in that the function silently skips its actual > function. > Is this what such a function should do? Or should it be "complain an > give up"? > >> { >> - strncpynt((char *)(buf->data + buf->capacity - len), str, >> len); >> + memcpy((void *)(buf->data + buf->capacity - len), (void >> *)str, (size_t)len); > > 2. This opens the door for another compiler warning, which is that you > are casting away the const-ness of "str". It is const char * when passed > in, but you cast it to void * (non-const). The 2nd argument of memcpy > would need to be cast to (const void *) to avoid that. memcpy is > declared (..., const void *restrict src, ...), so that's safe. > > 3. However, strncpynt would set buf->data[buf->capacity] = '\0', which > is now missing. I thin this is not needed in this case, because str [0..strlen(str) + 1] includes the \0 which will be copied by memcpy. No? > > So: NAK, please revise on #3. > > Cheers, > Matthias > > > > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Am 28.03.23 um 20:58 schrieb Antonio Quartulli: > Hi, > > On 28/03/2023 20:51, Matthias Andree wrote: >> Am 28.03.23 um 17:12 schrieb Frank Lichtenheld: >>> Since we use strlen() to determine the length >>> and then check it ourselves, there is really >>> no point in using strncpy. >>> >>> But the compiler might complain that we use >>> the output of strlen() for the length of >>> strncpy which is usually a sign for bugs: >>> >>> error: ‘strncpy’ specified bound depends >>> on the length of the source argument >>> [-Werror=stringop-overflow=] >>> >>> Warning was at least triggered for >>> mingw-gcc version 10-win32 20220113. >>> >>> Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com> >>> --- >>> src/openvpn/buffer.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c >>> index d099795b..de40d690 100644 >>> --- a/src/openvpn/buffer.c >>> +++ b/src/openvpn/buffer.c >>> @@ -317,9 +317,9 @@ buf_catrunc(struct buffer *buf, const char *str) >>> if (buf_forward_capacity(buf) <= 1) >>> { >>> int len = (int) strlen(str) + 1; >>> - if (len < buf_forward_capacity_total(buf)) >>> + if ((len > 0) && (len < buf_forward_capacity_total(buf))) >> >> I hope you don't mind but there are a few things I would like to bring >> to attention. >> >> 1. The first is clamping and wrapping around: >> >> strlen(someptr) returns size_t, and this can be >= INT_MAX. Add +1 and >> this wraps to INT_MIN. >> While you now trap this with the new "len > 0", there does not seem to >> be any error handling, in that the function silently skips its actual >> function. >> Is this what such a function should do? Or should it be "complain an >> give up"? >> >>> { >>> - strncpynt((char *)(buf->data + buf->capacity - len), >>> str, len); >>> + memcpy((void *)(buf->data + buf->capacity - len), (void >>> *)str, (size_t)len); >> >> 2. This opens the door for another compiler warning, which is that you >> are casting away the const-ness of "str". It is const char * when passed >> in, but you cast it to void * (non-const). The 2nd argument of memcpy >> would need to be cast to (const void *) to avoid that. memcpy is >> declared (..., const void *restrict src, ...), so that's safe. >> >> 3. However, strncpynt would set buf->data[buf->capacity] = '\0', which >> is now missing. > > I thin this is not needed in this case, because str [0..strlen(str) + > 1] includes the \0 which will be copied by memcpy. No? See my earlier consideration of the wrapping of the INT_MAX, when the copy comes out short. It's not making matters worse, but I'm not a friend of silent failure.
diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c index d099795b..de40d690 100644 --- a/src/openvpn/buffer.c +++ b/src/openvpn/buffer.c @@ -317,9 +317,9 @@ buf_catrunc(struct buffer *buf, const char *str) if (buf_forward_capacity(buf) <= 1) { int len = (int) strlen(str) + 1; - if (len < buf_forward_capacity_total(buf)) + if ((len > 0) && (len < buf_forward_capacity_total(buf))) { - strncpynt((char *)(buf->data + buf->capacity - len), str, len); + memcpy((void *)(buf->data + buf->capacity - len), (void *)str, (size_t)len); } } }
Since we use strlen() to determine the length and then check it ourselves, there is really no point in using strncpy. But the compiler might complain that we use the output of strlen() for the length of strncpy which is usually a sign for bugs: error: ‘strncpy’ specified bound depends on the length of the source argument [-Werror=stringop-overflow=] Warning was at least triggered for mingw-gcc version 10-win32 20220113. Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com> --- src/openvpn/buffer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)