[
https://issues.apache.org/jira/browse/PROTON-2517?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17506354#comment-17506354
]
ASF GitHub Bot commented on PROTON-2517:
----------------------------------------
astitcher commented on a change in pull request #362:
URL: https://github.com/apache/qpid-proton/pull/362#discussion_r826180173
##########
File path: c/src/core/emitters.h
##########
@@ -543,22 +544,27 @@ static inline void emit_multiple(pni_emitter_t* emitter,
pni_compound_context* c
emit_accumulated_nulls(emitter, compound);
if (!data || pn_data_size(data) == 0) {
pni_emitter_writef8(emitter, PNE_NULL);
Review comment:
Hmm, not related to your changes, but I wonder if this sequence is
correct - it looks like in the case where we would emit a null here we
shouldn't always emit the accumulated nulls first but just add another one. So
I think I got this in the wrong order with a missing early return.
##########
File path: c/src/core/emitters.h
##########
@@ -543,22 +544,27 @@ static inline void emit_multiple(pni_emitter_t* emitter,
pni_compound_context* c
emit_accumulated_nulls(emitter, compound);
if (!data || pn_data_size(data) == 0) {
pni_emitter_writef8(emitter, PNE_NULL);
- } else if (pn_data_type(data) == PN_ARRAY) {
- switch (pn_data_get_array(data)) {
- case 0:
- pni_emitter_writef8(emitter, PNE_NULL);
- break;
- case 1:
- pn_data_enter(data);
- pn_data_narrow(data);
- pni_emitter_data(emitter, data);
- pn_data_widen(data);
- break;
- default:
+ } else {
+ // Rewind and point to first node so data type is defined.
+ pn_data_rewind(data);
+ pn_data_next(data);
+ if (pn_data_type(data) == PN_ARRAY) {
+ switch (pn_data_get_array(data)) {
+ case 0:
+ pni_emitter_writef8(emitter, PNE_NULL);
+ break;
+ case 1:
+ pn_data_enter(data);
+ pn_data_narrow(data);
+ pni_emitter_data(emitter, data);
+ pn_data_widen(data);
+ break;
+ default:
+ pni_emitter_data(emitter, data);
+ }
+ } else {
Review comment:
Rather than re indent everything I'd prefer there to be an early return
and duplicate compound->count++ in the null case with no else block.
I should probably written it this way initially.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> The new C codec can misinterpret pn_data_t values resulting in unintended
> wire data.
> ------------------------------------------------------------------------------------
>
> Key: PROTON-2517
> URL: https://issues.apache.org/jira/browse/PROTON-2517
> Project: Qpid Proton
> Issue Type: Bug
> Components: proton-c
> Affects Versions: proton-c-0.37.0
> Reporter: Clifford Jansen
> Assignee: Clifford Jansen
> Priority: Major
>
> See the C++ frame trace from
> https://issues.redhat.com/browse/ENTMQCL-3278
> The zero length array is printed instead of a null because the test in
> emit_multiple() from emitters.h fails to set the current node of the
> pn_data_t to the first node. The test
> if (pn_data_type(data) == PN_ARRAY) { //...
> fails and the array processing logic is bypassed, including the lines
> switch (pn_data_get_array(data)) {
> case 0:
> pni_emitter_writef8(emitter, PNE_NULL);
>
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
