Uploaded image for project: 'Qpid Proton'
  1. Qpid Proton
  2. PROTON-1573

Undefined behavior in some calls to memmove in Proton

    XMLWordPrintableJSON

    Details

    • 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
    • Labels:
      None

      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

        Attachments

          Activity

            People

            • Assignee:
              aconway Alan Conway
              Reporter:
              jdanek Jiri Daněk
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: