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 <[email protected]> > --- > 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,
--
Antonio Quartulli
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
