event_rabbitmq: fix dupl_string() NUL-inclusive len corrupting AMQP shortstr#3834
Open
NormB wants to merge 1 commit intoOpenSIPS:masterfrom
Open
event_rabbitmq: fix dupl_string() NUL-inclusive len corrupting AMQP shortstr#3834NormB wants to merge 1 commit intoOpenSIPS:masterfrom
NormB wants to merge 1 commit intoOpenSIPS:masterfrom
Conversation
…hortstr dupl_string() incremented dst->len after NUL-terminating the unescaped string, causing .len to include the trailing NUL byte. This made amqp_basic_publish() encode exchange and routing-key shortstr fields with an extra 0x00, breaking broker routing. Remove the len++ and all downstream compensations (tls_dom_name.len--, and the - 1 adjustments in rmq_print() for address, exchange, routing key, and user). Also fix the un_escape() error path to free the already-allocated shm buffer, and fix the default-user allocation to explicitly NUL-terminate. Closes OpenSIPS#3828
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
dupl_string()inevent_rabbitmq.cincrementsdst->lenafter NUL-terminating the unescaped string (dst->len++), causing.lento include the trailing NUL byte. This makesamqp_basic_publish()encode exchange and routing-key as AMQP shortstr fields with an extra0x00byte, breaking broker routing.The bug was introduced in commit
0260de0c1(2014) whendupl_string()was refactored to useun_escape(). The originalmemcpy-based implementation had the same off-by-one (dst->len = end - begin + 1).Details
AMQP 0-9-1 encodes exchange and routing-key as shortstr fields (1-byte length prefix + N content bytes). With the inflated
.len, the broker receives a shortstr containing the field value followed by0x00, which fails to match the declared exchange or creates one with an embedded NUL.Over time, several
- 1compensations were added downstream to work around the inflated length:tls_dom_name.len--inrmq_parse()(commit472ae546c, TLS support)address.len - 1,exchange.len - 1,routing_key.len - 1inrmq_print()strlen(user) - 1inrmq_print()— this one was wrong even pre-patch sincestrlen()never counts the NULSolution
Remove the
dst->len++indupl_string()and all downstream compensations. Three additional issues fixed while testing:dupl_string()callsshm_mallocbeforeun_escape(), but onun_escape()failure the buffer was never freed. Addedshm_free+ cleanup.shm_malloc(rmq_static_holder.len)+memcpy(..., len)for"guest"didn't copy the NUL terminator. The pointer is passed toamqp_login()→strlen(). Changed tolen + 1.strlen(user) - 1: The/* skip 0 */comment reveals the author believedstrlen()includes the NUL. It doesn't — this always truncated the last character of the username inrmq_print()output.Tested with:
Compatibility
No backward compatibility issues. All behavioral changes are strictly correctional:
rmq_match(): symmetric — both sides were inflated equally, no changermq_print(): compensations removed, output unchangedamqp_login()user/password: useschar*viastrlen(), unaffectedClosing issues
Closes #3828