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] [Created] (PROTON-1573) Undefined behavior in some calls to memmove in Proton
Date Tue, 05 Sep 2017 20:48:00 GMT
Jiri Danek created PROTON-1573:

             Summary: 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

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.

SUMMARY: AddressSanitizer: undefined-behavior /home/jdanek/Work/repos/qpid-proton/proton-c/src/core/codec.c:268:35
/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

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.

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 {

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.

This message was sent by Atlassian JIRA

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

View raw message