MLE-28100 MLE-29561: fix ResourceSequence leak in ContentOutputFormat initialize() and null pointer dereference in NodeImpl.isEqualNode()#605
Open
vshaniga wants to merge 1 commit into
Conversation
….initialize() and null pointer dereference in NodeImpl.isEqualNode()
There was a problem hiding this comment.
Pull request overview
Fixes two correctness issues in mlcp’s Java code: (1) properly closing XCC ResultSequence created during ContentOutputFormat.initialize() to avoid leaking server-side resources, and (2) correcting guard logic and comparisons in NodeImpl.isEqualNode() to prevent NPEs and incorrect equality results.
Changes:
- Wrap
session.submitRequest(query)/ result parsing inContentOutputFormat.initialize()withtry/finallyto always closeResultSequence. - Fix
NodeImpl.isEqualNode()to use the correcthasAttributes()/hasChildNodes()guards and negate mismatch comparisons.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/main/java/com/marklogic/mapreduce/ContentOutputFormat.java | Adds try/finally to close ResultSequence and refactors initialization result parsing. |
| src/main/java/com/marklogic/dom/NodeImpl.java | Fixes equality logic to avoid NPE and correct boolean comparison semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+433
to
+441
| } else { | ||
| // check if input needs to be in HTTP compliant mode | ||
| String inputRestrictHost = conf.getTrimmed(INPUT_RESTRICT_HOSTS); | ||
| if (inputRestrictHost == null || | ||
| inputRestrictHost.equalsIgnoreCase("false")) { | ||
| HttpChannel.setUseHTTP(false); | ||
| if (LOG.isDebugEnabled()) { | ||
| LOG.debug("HTTP compliant mode disabled since x-forwarded-for doesn't exist"); | ||
| } |
Comment on lines
+485
to
504
| // initialize fastload mode | ||
| if (conf.get(OUTPUT_FAST_LOAD) == null) { | ||
| // fastload not set | ||
| if (conf.get(OUTPUT_DIRECTORY) != null) { | ||
| // output_dir is set, attempt to do fastload | ||
| if(conf.get(OUTPUT_PARTITION) == null && | ||
| (policy == AssignmentPolicy.Kind.RANGE || | ||
| policy == AssignmentPolicy.Kind.QUERY)) { | ||
| fastLoad = false; | ||
| } else if (policy == AssignmentPolicy.Kind.RANGE || | ||
| policy == AssignmentPolicy.Kind.QUERY || | ||
| policy == AssignmentPolicy.Kind.STATISTICAL) { | ||
| fastLoad = allowFastLoad; | ||
| } else { | ||
| fastLoad = true; | ||
| } | ||
| } else { | ||
| fastLoad = true; | ||
| //neither fastload nor output_dir is set | ||
| fastLoad = false; | ||
| } |
Comment on lines
312
to
348
| public boolean isEqualNode(Node other) { | ||
|
|
||
| // Note that normalization can affect equality; to avoid this, | ||
| // nodes should be normalized before being compared. | ||
| // For the moment, normalization cannot be done. | ||
| if (other==null) return false; | ||
| if (getNodeType() != other.getNodeType()) | ||
| return false; | ||
| if (notequals(getLocalName(),other.getLocalName())) | ||
| return false; | ||
| if (notequals(getNamespaceURI(),other.getNamespaceURI())) | ||
| return false; | ||
| if (notequals(getPrefix(), other.getPrefix())) | ||
| return false; | ||
| if (notequals(getNodeValue(),other.getNodeValue())) | ||
| return false; | ||
| if (hasChildNodes() != other.hasChildNodes()) | ||
| return false; | ||
| if (hasAttributes() != other.hasAttributes()) | ||
| return false; | ||
| if (hasChildNodes()) { | ||
| if (hasAttributes()) { | ||
| NamedNodeMap thisAttr = getAttributes(); | ||
| NamedNodeMap otherAttr = other.getAttributes(); | ||
| if (thisAttr.getLength() != otherAttr.getLength()) | ||
| return false; | ||
| for (int i = 0; i < thisAttr.getLength(); i++) | ||
| if (thisAttr.item(i).isEqualNode(otherAttr.item(i))) | ||
| if (!thisAttr.item(i).isEqualNode(otherAttr.item(i))) | ||
| return false; | ||
| } | ||
| if (hasAttributes()) { | ||
| if (hasChildNodes()) { | ||
| NodeList thisChild = getChildNodes(); | ||
| NodeList otherChild = other.getChildNodes(); | ||
| if (thisChild.getLength() != otherChild.getLength()) | ||
| return false; | ||
| for (int i = 0; i < thisChild.getLength(); i++) | ||
| if (thisChild.item(i).isEqualNode(otherChild.item(i))) | ||
| if (!thisChild.item(i).isEqualNode(otherChild.item(i))) | ||
| return false; |
Collaborator
Author
|
I just added a try/finally around the result handling and indented the inner code accordingly. whitespace-ignored diff: https://github.com/marklogic/marklogic-contentpump/pull/605/changes?w=1 |
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.
MLE-28100 — Fix
ResultSequenceleak inContentOutputFormat.initialize()Problem:
initialize()calledsession.submitRequest(query)and stored the result in aResultSequence, but never closed it. On repeated initializations or exception paths, this leaked server-side connections and sockets.Fix:
submitRequestcall and result-reading logic in a try/finally block that callsresult.close().File: src/main/java/com/marklogic/mapreduce/ContentOutputFormat.java
MLE-29561 — Fix NULL pointer dereference and logic bugs in
NodeImpl.isEqualNode()Problem:
Three logic bugs in
isEqualNode():hasChildNodes()guard was used to enter the attributes comparison block —getAttributes()could return null if the node has no attributes, causing a NPE.hasAttributes()guard was used to enter the child nodes comparison block — incorrect semantics.isEqualNode()comparisons were missing negation — the method returned false when nodes were equal instead of when they were not equal.Fix:
hasAttributes() / hasChildNodes()guards to match their respective blocks.isEqualNode()comparisons.File: src/main/java/com/marklogic/dom/NodeImpl.java
Tests