[jira] [Commented] (PROTON-1573) Undefined behavior in some calls to memmove in Proton
[
https://issues.apache.org/jira/browse/PROTON-1573?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16192912#comment-16192912
]
Justin Ross commented on PROTON-1573:
-
Thanks, [~jdanek].
> 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 Daněk
>Assignee: Alan Conway
>Priority: Trivial
> Fix For: proton-c-0.18.0
>
>
> 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([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
[jira] [Commented] (PROTON-1573) Undefined behavior in some calls to memmove in Proton
[
https://issues.apache.org/jira/browse/PROTON-1573?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16192810#comment-16192810
]
Jiri Daněk commented on PROTON-1573:
This one here
https://github.com/apache/qpid-proton/commit/feafb6c80b121f0a7ce87ea09c9b2f0f2d0fadf5
> 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 Daněk
>Assignee: Alan Conway
>Priority: Trivial
> Fix For: proton-c-0.18.0
>
>
> 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([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
[jira] [Commented] (PROTON-1573) Undefined behavior in some calls to memmove in Proton
[
https://issues.apache.org/jira/browse/PROTON-1573?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16170140#comment-16170140
]
Jiri Danek commented on PROTON-1573:
I think this is resolved already. I'll look for the specific commit.
> 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([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
