Conversation
… message starts with segment delimiter Guard against i == 0 before accessing hl7MessageBytes[i - 1] in populateHl7DataHeaders, and treat endOfMSH == 0 as invalid (same as -1) so a zero-length MSH does not fall through to the header parsing branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
oscerd
left a comment
There was a problem hiding this comment.
Correct fix, and a good one to make since MllpTcpServerConsumer parses untrusted network input — a malformed payload should never crash header population.
The two changes are consistent. When the first byte is a segment delimiter, the loop enters the SEGMENT_DELIMITER branch at i == 0; the i > 0 guard short-circuits before hl7MessageBytes[i - 1], so the ArrayIndexOutOfBoundsException: Index -1 is avoided, fieldSeparatorIndexes is left untouched, and endOfMSH is set to 0. The follow-on check then becomes endOfMSH <= 0, which folds the new zero-length-MSH case (delimiter at byte 0 → endOfMSH == 0) in with the existing not-found case (-1), so both go to the "unable to find the end of the MSH segment" warning and header parsing runs only for a real, non-empty MSH (endOfMSH > 0). Empty input and a one-byte delimiter-only message both land in the warn branch now instead of throwing.
The new MllpTcpServerConsumerPopulateHl7DataHeadersTest covers the reported crash (leading delimiter) and the fieldSeparator == SEGMENT_DELIMITER edge, and the full MLLP suite passes.
LGTM. This is a fork PR so CI has not run yet; I'll hold the formal approval until the workflow is authorized and the checks are green.
This review was generated with AI assistance and reviewed/issued by the human operator. Claude Code on behalf of oscerd
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @tmielke, this is a clean, minimal fix for a long-standing edge case (the loop dates back to the 2018 rewrite).
Verified locally (PR head 73ed411):
MllpTcpServerConsumerPopulateHl7DataHeadersTestandMllpTcpServerConsumerMessageHeadersTestpass.- With only the production change reverted,
testMessageStartingWithSegmentDelimiterfails withArrayIndexOutOfBoundsException: Index -1, so the test does reproduce the reported bug. - No formatter/impsort changes after the build.
What changes for users: before this fix, the AIOOBE was caught by the outer catch in processMessage and reported as the misleading "Unexpected exception creating Unit of Work". The exchange never reached the route and no ACK was sent. Now the message reaches the route without HL7 headers, a WARN is logged, and auto-ack goes through Hl7Util.generateAcknowledgementPayload, which already rejects this payload with a controlled MllpAcknowledgementGenerationException (the MSH has fewer than 8 fields). That is the graceful handling the JIRA asks for.
Minor, non-blocking notes on the test are inline. On its own, the i > 0 guard would already stop the crash: with endOfMSH == 0 the else branch finds no field indexes and sets no headers. The endOfMSH <= 0 change is still worthwhile because it logs the WARN for this malformed case.
CI workflows are waiting for maintainer approval (action_required), so there are no check results yet.
This is a rules-and-conventions review. It does not replace specialized AI review tools or static analysis such as SonarCloud.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of davsclaus
|
|
||
| @Override | ||
| protected void doPreSetup() throws Exception { | ||
| MllpComponent mllpComponent = createCamelContext().getComponent("mllp", MllpComponent.class); |
There was a problem hiding this comment.
Nit: createCamelContext() here builds a second CamelContext that is never started or stopped. The consumer and endpoint then belong to that orphan context, while the exchanges in the tests are created on the managed context. It works, but it is a bit surprising. You could build the consumer lazily in the test methods from context.getEndpoint("mllp://localhost:0", MllpEndpoint.class), or drop CamelTestSupport and use a plain DefaultCamelContext for this pure unit test.
| } | ||
|
|
||
| @Test | ||
| void testFieldSeparatorEqualsSegmentDelimiter() { |
There was a problem hiding this comment.
Note: this case passes on the code before the fix as well. When fieldSeparator == SEGMENT_DELIMITER, the else if branch is never reached and endOfMSH stays -1, so it does not exercise either of the two changed lines. It is fine to keep as a guard against regressions. You could say so in the comment, or add a case that hits endOfMSH == 0 without the i - 1 access.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 of 697 tested, 27 compile-only — current: 9 all testedMaveniverse Scalpel detected 9 affected modules (current approach: 9). Skip-tests mode would test 9 modules (1 direct + 8 downstream), skip tests for 27 (generated code, meta-modules) Modules Scalpel would test (9)
Modules with tests skipped (27)
All tested modules (36 modules, 5m 10s total)Total reactor time: 5m 10s
Top 20 slowest modules:
|
allthingssecurity
left a comment
There was a problem hiding this comment.
Thanks @tmielke. I checked this independently and it looks good to me.
- With main's
MllpTcpServerConsumer,testMessageStartingWithSegmentDelimiterfails withArrayIndexOutOfBoundsException: Index -1. With the PR it passes. The whole camel-mllp suite passes (350 tests, 3 skipped). - End to end over a socket,
\rMSH|^~\&|ADT|...now reaches the route (WARN "unable to find the end of the MSH segment"). The exchange then fails with the controlledMllpAcknowledgementGenerationExceptionfrom auto-ack instead of the AIOOBE. On main the route never saw the message.
One related thing I found while testing. It is not caused by this PR and could be a separate JIRA:
Hl7Util.generateAcknowledgementPayload checks fieldSeparatorIndexes.size() < 8, but then reads get(8) and get(9) (its own message says "10 are required"). So an MSH with 8 or 9 field-separator positions still fails with an unchecked exception. MSH|^~\&|A|B|C|D|20160902123950| gives IndexOutOfBoundsException: Index: 8, Size: 8, and MSH|^~\&|A|B|C|D|20160902123950|| gives Index: 9, Size: 9. On the consumer side this escapes the catch (MllpAcknowledgementGenerationException) in MllpTcpServerConsumer, and the connection is closed without an ACK. Changing the guard to < 10 would turn it into the same controlled exception as here. I am happy to open the JIRA if you agree.
Claude Code on behalf of allthingssecurity
Description
When
MllpTcpServerConsumerreceives an MLLP payload whose first byte is a segment delimiter (0x0D / \r), populateHl7DataHeaders crashes withArrayIndexOutOfBoundsException: Index -1because the parsing loop accesseshl7MessageBytes[i - 1]at i == 0.This is a latent defect in the original 2018 implementation — the default configuration (hl7Headers=true, validatePayload=false) leaves this path unguarded.
Changes
MllpTcpServerConsumer.java— two fixes:i > 0guard beforehl7MessageBytes[i - 1]to prevent the out-of-bounds access-1 == endOfMSH) to if (endOfMSH <= 0) so a zero-length MSH segment (delimiter at byte 0) is treated as invalid rather than falling through to the header parsing branchMllpTcpServerConsumerPopulateHl7DataHeadersTest.java— new unit test with two test cases:ieldSeparator == SEGMENT_DELIMITER(edge case where byte[3] is also 0x0D)Test plan
mvn test -Dtest=MllpTcpServerConsumerPopulateHl7DataHeadersTest -pl components/camel-mllp)Target
mainbranch)Tracking
Apache Camel coding standards and style
I checked that each commit in the pull request has a meaningful subject line and body.
I have run
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.AI-assisted contributions
Co-authored-bytrailers) and the PR description identifies the AI tool used.