[
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-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
