[Openvpn-devel,v2] Copy one byte less in strncpynt()

Message ID 20190608075622.11589-1-gert@greenie.muc.de
State Accepted
Headers show
Series
  • [Openvpn-devel,v2] Copy one byte less in strncpynt()
Related show

Commit Message

Gert Doering June 8, 2019, 7:56 a.m.
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(-)

Comments

Antonio Quartulli June 8, 2019, 5:51 p.m. | #1
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>
Gert Doering June 9, 2019, 9:03 a.m. | #2
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

Patch

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;
     }
 }