qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jiri Danek (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (PROTON-1573) Undefined behavior in some calls to memmove in Proton
Date Tue, 05 Sep 2017 20:50:00 GMT

     [ https://issues.apache.org/jira/browse/PROTON-1573?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Jiri Danek updated PROTON-1573:
-------------------------------
    Description: 
Proton sometimes calls to memmove with arguments memmove(?, null, 0), that is, the source
pointer is null and length is zero.

According to UndefinedBehaviorSanitizer tool (UBSan), this has undefined behavior.

{noformat}
SUMMARY: AddressSanitizer: undefined-behavior /home/jdanek/Work/repos/qpid-proton/proton-c/src/core/codec.c:268:35
in 
/home/jdanek/Work/repos/qpid-proton/proton-c/src/core/framing.c:97:39: runtime error: null
pointer passed as argument 2, which is declared to never be null
{noformat}

It can be "fixed" in the following manner, but I think it is probably not worth worrying about,
unless the code can be somehow restructured that {{n = 0}} is caught sooner. I verified the
fix by running UBSan again. Nothing was reported this time.

{noformat}
diff --git a/proton-c/src/core/buffer.c b/proton-c/src/core/buffer.c
index c3015f49..f47acd6d 100644
--- a/proton-c/src/core/buffer.c
+++ b/proton-c/src/core/buffer.c
@@ -170,8 +170,12 @@ int pn_buffer_append(pn_buffer_t *buf, const char *bytes, size_t size)
   size_t tail_space = pni_buffer_tail_space(buf);
   size_t n = pn_min(tail_space, size);
 
+  if (n > 0) {
   memmove(buf->bytes + tail, bytes, n);
+  }
+  if (size - n > 0) { 
   memmove(buf->bytes, bytes + n, size - n);
+  }
 
   buf->size += size;
 
diff --git a/proton-c/src/core/framing.c b/proton-c/src/core/framing.c
index 09bf4bba..d2c355f8 100644
--- a/proton-c/src/core/framing.c
+++ b/proton-c/src/core/framing.c
@@ -94,7 +94,9 @@ size_t pn_write_frame(char *bytes, size_t available, pn_frame_t frame)
     bytes[5] = frame.type;
     pn_i_write16(&bytes[6], frame.channel);
 
-    memmove(bytes + AMQP_HEADER_SIZE, frame.extended, frame.ex_size);
+    if (frame.ex_size > 0) {
+        memmove(bytes + AMQP_HEADER_SIZE, frame.extended, frame.ex_size);
+    }
     memmove(bytes + 4*doff, frame.payload, frame.size);
     return size;
   } else {
{noformat}

After brief Googling, I believe that although this really is an undefined behavior, it is
reasonable to expect that any real-world implementation of memmove will be a simple noop as
long as {{n = 0}}, which it is always the case. (https://stackoverflow.com/a/5243068/1047788)

Probably best to ignore this.

The tests that uncover this are for example

proton_tests.engine.ConnectionTest.test_capabilities:
../proton-c/src/core/framing.c:97:5: runtime error: null pointer passed as argument 2, which
is declared to never be null

proton_tests.engine.CreditTest.testPartialDrain:
../proton-c/src/core/buffer.c:173:3: runtime error: null pointer passed as argument 2, which
is declared to never be null
../proton-c/src/core/buffer.c:174:3: runtime error: null pointer passed as argument 2, which
is declared to never be null


  was:
Proton sometimes calls to memmove with arguments memmove(?, null, 0), that is, the source
pointer is null and length is zero.

According to UndefinedBehaviorSanitizer tool (UBSan), this has undefined behavior.

{noformat}
SUMMARY: AddressSanitizer: undefined-behavior /home/jdanek/Work/repos/qpid-proton/proton-c/src/core/codec.c:268:35
in 
/home/jdanek/Work/repos/qpid-proton/proton-c/src/core/framing.c:97:39: runtime error: null
pointer passed as argument 2, which is declared to never be null
{noformat}

It can be "fixed" in the following manner, but I think it is probably not worth it. I verified
the fix by running UBSan again. Nothing was reported this time.

{noformat}
diff --git a/proton-c/src/core/buffer.c b/proton-c/src/core/buffer.c
index c3015f49..f47acd6d 100644
--- a/proton-c/src/core/buffer.c
+++ b/proton-c/src/core/buffer.c
@@ -170,8 +170,12 @@ int pn_buffer_append(pn_buffer_t *buf, const char *bytes, size_t size)
   size_t tail_space = pni_buffer_tail_space(buf);
   size_t n = pn_min(tail_space, size);
 
+  if (n > 0) {
   memmove(buf->bytes + tail, bytes, n);
+  }
+  if (size - n > 0) { 
   memmove(buf->bytes, bytes + n, size - n);
+  }
 
   buf->size += size;
 
diff --git a/proton-c/src/core/framing.c b/proton-c/src/core/framing.c
index 09bf4bba..d2c355f8 100644
--- a/proton-c/src/core/framing.c
+++ b/proton-c/src/core/framing.c
@@ -94,7 +94,9 @@ size_t pn_write_frame(char *bytes, size_t available, pn_frame_t frame)
     bytes[5] = frame.type;
     pn_i_write16(&bytes[6], frame.channel);
 
-    memmove(bytes + AMQP_HEADER_SIZE, frame.extended, frame.ex_size);
+    if (frame.ex_size > 0) {
+        memmove(bytes + AMQP_HEADER_SIZE, frame.extended, frame.ex_size);
+    }
     memmove(bytes + 4*doff, frame.payload, frame.size);
     return size;
   } else {
{noformat}

After brief Googling, I believe that although this really is an undefined behavior, it is
reasonable to expect that any real-world implementation of memmove will be a simple noop as
long as {{n = 0}}, which it is always the case. (https://stackoverflow.com/a/5243068/1047788)

Probably best to ignore this.

The tests that uncover this are for example

proton_tests.engine.ConnectionTest.test_capabilities:
../proton-c/src/core/framing.c:97:5: runtime error: null pointer passed as argument 2, which
is declared to never be null

proton_tests.engine.CreditTest.testPartialDrain:
../proton-c/src/core/buffer.c:173:3: runtime error: null pointer passed as argument 2, which
is declared to never be null
../proton-c/src/core/buffer.c:174:3: runtime error: null pointer passed as argument 2, which
is declared to never be null



> Undefined behavior in some calls to memmove in Proton
> -----------------------------------------------------
>
>                 Key: PROTON-1573
>                 URL: https://issues.apache.org/jira/browse/PROTON-1573
>             Project: Qpid Proton
>          Issue Type: Bug
>          Components: proton-c
>    Affects Versions: proton-c-0.18.0
>            Reporter: Jiri Danek
>            Priority: Trivial
>
> Proton sometimes calls to memmove with arguments memmove(?, null, 0), that is, the source
pointer is null and length is zero.
> According to UndefinedBehaviorSanitizer tool (UBSan), this has undefined behavior.
> {noformat}
> SUMMARY: AddressSanitizer: undefined-behavior /home/jdanek/Work/repos/qpid-proton/proton-c/src/core/codec.c:268:35
in 
> /home/jdanek/Work/repos/qpid-proton/proton-c/src/core/framing.c:97:39: runtime error:
null pointer passed as argument 2, which is declared to never be null
> {noformat}
> It can be "fixed" in the following manner, but I think it is probably not worth worrying
about, unless the code can be somehow restructured that {{n = 0}} is caught sooner. I verified
the fix by running UBSan again. Nothing was reported this time.
> {noformat}
> diff --git a/proton-c/src/core/buffer.c b/proton-c/src/core/buffer.c
> index c3015f49..f47acd6d 100644
> --- a/proton-c/src/core/buffer.c
> +++ b/proton-c/src/core/buffer.c
> @@ -170,8 +170,12 @@ int pn_buffer_append(pn_buffer_t *buf, const char *bytes, size_t
size)
>    size_t tail_space = pni_buffer_tail_space(buf);
>    size_t n = pn_min(tail_space, size);
>  
> +  if (n > 0) {
>    memmove(buf->bytes + tail, bytes, n);
> +  }
> +  if (size - n > 0) { 
>    memmove(buf->bytes, bytes + n, size - n);
> +  }
>  
>    buf->size += size;
>  
> diff --git a/proton-c/src/core/framing.c b/proton-c/src/core/framing.c
> index 09bf4bba..d2c355f8 100644
> --- a/proton-c/src/core/framing.c
> +++ b/proton-c/src/core/framing.c
> @@ -94,7 +94,9 @@ size_t pn_write_frame(char *bytes, size_t available, pn_frame_t frame)
>      bytes[5] = frame.type;
>      pn_i_write16(&bytes[6], frame.channel);
>  
> -    memmove(bytes + AMQP_HEADER_SIZE, frame.extended, frame.ex_size);
> +    if (frame.ex_size > 0) {
> +        memmove(bytes + AMQP_HEADER_SIZE, frame.extended, frame.ex_size);
> +    }
>      memmove(bytes + 4*doff, frame.payload, frame.size);
>      return size;
>    } else {
> {noformat}
> After brief Googling, I believe that although this really is an undefined behavior, it
is reasonable to expect that any real-world implementation of memmove will be a simple noop
as long as {{n = 0}}, which it is always the case. (https://stackoverflow.com/a/5243068/1047788)
> Probably best to ignore this.
> The tests that uncover this are for example
> proton_tests.engine.ConnectionTest.test_capabilities:
> ../proton-c/src/core/framing.c:97:5: runtime error: null pointer passed as argument 2,
which is declared to never be null
> proton_tests.engine.CreditTest.testPartialDrain:
> ../proton-c/src/core/buffer.c:173:3: runtime error: null pointer passed as argument 2,
which is declared to never be null
> ../proton-c/src/core/buffer.c:174:3: runtime error: null pointer passed as argument 2,
which is declared to never be null



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org


Mime
View raw message