| Message ID | 20190608075622.11589-1-gert@greenie.muc.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v2] Copy one byte less in strncpynt() | expand |
Hi, On 08/06/2019 09:56, 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. > > v2: do not ASSERT() on "maxlen == 0", but move the strncpy() call inside > the if() clause - so "just do nothing" on maxlen == 0, as before. > > Signed-off-by: Gert Doering <gert@greenie.muc.de> > --- > src/openvpn/buffer.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h > index a4fe6f9b..e8b83a5e 100644 > --- a/src/openvpn/buffer.h > +++ b/src/openvpn/buffer.h > @@ -347,9 +347,9 @@ 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); > if (maxlen > 0) > { > + strncpy(dest, src, maxlen-1); can you add spaces around the '-' when you commit this change? (basically like the line below) > dest[maxlen - 1] = 0; > } > } > Acked-by: Antonio Quartulli <a@unstable.cc>
Patch has been applied to the master branch.
(I have left the "maxlen-1" as it was, as uncrustify says this is
fine and I personally find "maxlen - 1" less readable here. I did fix
the tab-vs-spaces violation)
commit bebd25a0e3a2aba0b1f98463a87b24db6c419a66
Author: Gert Doering
Date: Sat Jun 8 09:56:22 2019 +0200
Copy one byte less in strncpynt()
Signed-off-by: Gert Doering <gert@greenie.muc.de>
Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20190608075622.11589-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg18502.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h index a4fe6f9b..e8b83a5e 100644 --- a/src/openvpn/buffer.h +++ b/src/openvpn/buffer.h @@ -347,9 +347,9 @@ 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); if (maxlen > 0) { + strncpy(dest, src, maxlen-1); 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. v2: do not ASSERT() on "maxlen == 0", but move the strncpy() call inside the if() clause - so "just do nothing" on maxlen == 0, as before. Signed-off-by: Gert Doering <gert@greenie.muc.de> --- src/openvpn/buffer.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)