diff mbox series

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

Message ID 20190228185128.43982-1-gert@greenie.muc.de
State Superseded
Headers
Series [Openvpn-devel] Copy one byte less in strncpynt() |

Commit Message

Gert Doering Feb. 28, 2019, 7:51 a.m. UTC 
  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(-)

Comments

Antonio Quartulli Feb. 28, 2019, 8:30 a.m. UTC | #1 
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 mbox series

Patch

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;