[PROTON-1573] Undefined behavior in some calls to memmove in Proton

• Type: Bug
• Status: Closed
• Priority: Trivial
• Resolution: Fixed
• Affects Version/s: proton-c-0.18.0
• Fix Version/s: proton-c-0.18.0
• Component/s: proton-c
• Assignee: Alan Conway
• Reporter: Jiri Daněk
• Created: 05/Sep/17 20:47
• Updated: 24/Oct/17 08:45
• Resolved: 05/Oct/17 14:02

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.

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

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.

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.

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