Skip to content

Fix ConcurrentModificationException in ISO components #717

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chhil wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix ConcurrentModificationException in ISO components #717

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e0a42fe
Fix ConcurrentModificationException in ISO components
chhil May 10, 2026
9f6f891
fix: unify ISOMsg synchronization on fields lock and document live re…
chhil May 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion jpos/src/main/java/org/jpos/iso/ISODatasetField.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,11 @@ public void unpack(InputStream in) throws ISOException {
public void dump(PrintStream p, String indent) {
p.println(indent + "<" + XMLPackager.ISOFIELD_TAG + " " + XMLPackager.ID_ATTR + "=\"" + fieldNumber + "\" type=\"dataset\">");
String innerIndent = indent + " ";
for (Dataset dataset : datasets) {
List<Dataset> snapshot;
synchronized (datasets) {
snapshot = new ArrayList<>(datasets);
}
for (Dataset dataset : snapshot) {
p.println(innerIndent + "<dataset id=\"" + String.format("%02X", dataset.getIdentifier()) + "\" format=\"" + dataset.getFormat() + "\">");
String datasetIndent = innerIndent + " ";
for (DatasetElement element : dataset.getElements()) {
Expand Down
26 changes: 20 additions & 6 deletions jpos/src/main/java/org/jpos/iso/ISOMsg.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,18 @@ public int getMaxField() {
recalcMaxField();
return maxField;
}

/**
* Returns a snapshot of field numbers as a new TreeSet.
* Safe for concurrent iteration during dump operations.
* TreeSet maintains sorted order required for XMLPackager.
* @return new TreeSet containing current field numbers
*/
public Set<Integer> getFieldNumbers() {
synchronized (fields) {
return new TreeSet<>(fields.keySet());
}
}
private void recalcMaxField() {
maxField = 0;
for (Object obj : fields.keySet()) {
Expand Down Expand Up @@ -232,11 +244,13 @@ public ISOPackager getPackager () {
*/
public void set (ISOComponent c) throws ISOException {
if (c != null) {
Integer i = (Integer) c.getKey();
fields.put (i, c);
if (i > maxField)
maxField = i;
dirty = true;
synchronized (fields) {
Integer i = (Integer) c.getKey();
fields.put (i, c);
if (i > maxField)
maxField = i;
dirty = true;
}
Comment on lines +279 to +286

@alcarraz alcarraz May 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this woun't make much of a difference, but why not putting the synchronized block just arround fields.put?

Suggested change
synchronized (fields) {
Integer i = (Integer) c.getKey();
fields.put (i, c);
if (i > maxField)
maxField = i;
dirty = true;
}
Integer i = (Integer) c.getKey();
synchronized (fields) {
fields.put (i, c);5
}
if (i > maxField)
maxField = i;
dirty = true;

@chhil chhil May 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative to this would be to just state that dump is not thread-safe.

We see CME's as warning in the log. At least for jpos components it should work, but user ones is not jpos responsibility.

but why not putting the synchronized block just arround fields.put?

My test were failing. I distinctly recall having to push the dirty flag inside too.

maxField must stay inside the synchronized block. It has no volatile keyword and no synchronization on read (only via recalcBitMap() → getMaxField()). If maxField = i moved outside the lock, another thread could enter getFieldNumbers() (which does acquire the lock) and call recalcBitMap(), which iterates fields.keySet() — but sees a stale maxField. This would cause the bitmap to be recalculated with an incorrect range, potentially missing fields.

@alcarraz alcarraz May 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but why not putting the synchronized block just arround fields.put?

My test were failing. I distinctly recall having to push the dirty flag inside too.

maxField must stay inside the synchronized block. It has no volatile keyword and no synchronization on read (only via recalcBitMap() → getMaxField()). If maxField = i moved outside the lock, another thread could enter getFieldNumbers() (which does acquire the lock) and call recalcBitMap(), which iterates fields.keySet() — but sees a stale maxField. This would cause the bitmap to be recalculated with an incorrect range, potentially missing fields.

Yes, I was thinking only about the CME when I wrote that. Since I didn't see any other place where synchronized(fields) was accessing the dirty flags. Shouldn't recalcBitMap also be synchronized on fields then? And unset, getMaxField?

}
}

Expand Down Expand Up @@ -654,7 +668,7 @@ public void dump (PrintStream p, String indent) {
if (header instanceof Loggeable)
((Loggeable) header).dump (p, newIndent);

for (int i : fields.keySet()) {
for (int i : getFieldNumbers()) {
//If you want the bitmap dumped in the log, change the condition from (i >= 0) to (i >= -1).
if (i >= 0) {
if ((c = (ISOComponent) fields.get(i)) != null)
Expand Down
10 changes: 8 additions & 2 deletions jpos/src/main/java/org/jpos/security/CryptographicServiceMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,9 @@ public void addField(String tag, String content) {
Objects.requireNonNull(tag, "The tag is required");
Objects.requireNonNull(content, "The content is required");
tag = tag.toUpperCase();
fields.put(tag, content);
synchronized (fields) {
fields.put(tag, content);
}
}

/**
Expand Down Expand Up @@ -191,7 +193,11 @@ public void dump (PrintStream p, String indent) {
p.print(indent + "<csm");
p.print(" class=\"" + getMCL() + "\"");
p.println(">");
for (String tag : fields.keySet()) {
List<String> snapshot;
synchronized (fields) {
snapshot = new ArrayList<>(fields.keySet());
}
for (String tag : snapshot) {
p.println(inner + "<field tag=\"" + tag + "\" value=\"" + getFieldContent(tag) + "\"/>");
}
p.println(indent + "</csm>");
Expand Down
10 changes: 8 additions & 2 deletions jpos/src/main/java/org/jpos/security/SecureKeyBlock.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@
package org.jpos.security;

import java.io.PrintStream;
import java.util.ArrayList;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import org.jpos.iso.ISOUtil;
Expand Down Expand Up @@ -280,9 +282,13 @@ public void dump(PrintStream p, String indent) {
p.println(inner2 + "<reserved>" + reserved + "</reserved>");
p.println(inner + "</header>");

if (!optionalHeaders.isEmpty()) {
List<Entry<String, String>> snapshot;
synchronized (this) {
snapshot = new ArrayList<>(optionalHeaders.entrySet());
}
if (!snapshot.isEmpty()) {
p.println(inner + "<optional-header>");
for (Entry<String, String> ent : optionalHeaders.entrySet())
for (Entry<String, String> ent : snapshot)
p.println(inner2 + "<entry id=\""+ ent.getKey() + "\" value=\""+ ent.getValue()+ "\"/>");
p.println(inner + "</optional-header>");
}
Expand Down
8 changes: 7 additions & 1 deletion jpos/src/main/java/org/jpos/security/SecureKeySpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@
import org.jpos.util.Loggeable;

import java.io.Serializable;
import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import org.jpos.iso.ISOUtil;
Expand Down Expand Up @@ -511,7 +513,11 @@ public void dump(PrintStream p, String indent) {
if (!optionalHeaders.isEmpty()) {
p.println(inner + "<optional-header>");
String inner2 = inner + " ";
for (Entry<String, String> ent : optionalHeaders.entrySet())
List<Entry<String, String>> snapshot;
synchronized (optionalHeaders) {
snapshot = new ArrayList<>(optionalHeaders.entrySet());
}
for (Entry<String, String> ent : snapshot)
p.println(inner2 + "<entry id=\""+ ent.getKey() + "\" value=\""+ ent.getValue()+ "\"/>");
p.println(inner + "</optional-header>");
}
Expand Down
26 changes: 18 additions & 8 deletions jpos/src/main/java/org/jpos/tlv/TLVList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,9 @@ public void unpack(byte[] buf, int offset) throws IllegalArgumentException
public void append(TLVMsg tlv) throws NullPointerException {
Objects.requireNonNull(tlv, "TLV message cannot be null");

tags.add(tlv);
synchronized (tags) {
tags.add(tlv);
}
}

/**
Expand Down Expand Up @@ -250,20 +252,24 @@ public TLVList append(int tag, String value) throws IllegalArgumentException {
* @param index number
*/
public void deleteByIndex(int index) {
tags.remove(index);
synchronized (tags) {
tags.remove(index);
}
}

/**
* Delete the specified TLV from the list by tag value
* @param tag id
*/
public void deleteByTag(int tag) {
List<TLVMsg> t = new ArrayList<>();
for (TLVMsg tlv2 : tags) {
if (tlv2.getTag() == tag)
t.add(tlv2);
synchronized (tags) {
List<TLVMsg> t = new ArrayList<>();
for (TLVMsg tlv2 : tags) {
if (tlv2.getTag() == tag)
t.add(tlv2);
}
tags.removeAll(t);
}
tags.removeAll(t);
}

/**
Expand Down Expand Up @@ -536,7 +542,11 @@ public boolean hasTag(int tag) {
public void dump(PrintStream p, String indent) {
String inner = indent + " ";
p.println(indent + "<tlvlist>");
for (TLVMsg msg : getTags())
List<TLVMsg> snapshot;
synchronized (tags) {
snapshot = new ArrayList<>(tags);
}
for (TLVMsg msg : snapshot)
msg.dump(p, inner);
p.println(indent + "</tlvlist>");
}
Expand Down
18 changes: 13 additions & 5 deletions jpos/src/main/java/org/jpos/util/FSDMsg.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@
import java.io.PrintStream;
import java.net.URL;
import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Objects;
Expand Down Expand Up @@ -652,10 +654,12 @@ protected String readField (InputStreamReader r, String fieldName, int len,
* @param value the field value, or null to remove
*/
public void set (String name, String value) {
if (value != null)
fields.put (name, value);
else
fields.remove (name);
synchronized (fields) {
if (value != null)
fields.put (name, value);
else
fields.remove (name);
}
}
/**
* Sets the binary header bytes for this message.
Expand Down Expand Up @@ -894,7 +898,11 @@ public void dump (PrintStream p, String indent) {
if (header != null) {
append (p, "header", getHexHeader(), inner);
}
for (String f :fields.keySet())
List<String> snapshot;
synchronized (fields) {
snapshot = new ArrayList<>(fields.keySet());
}
for (String f : snapshot)
append (p, f, fields.get (f), inner);
p.println (indent + "</fsdmsg>");
}
Expand Down
8 changes: 4 additions & 4 deletions jpos/src/main/java/org/jpos/util/SimpleMsg.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.OutputStream;

import org.jpos.iso.ISOUtil;

import java.io.PrintStream;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;

import org.jpos.iso.ISOUtil;

/**
* <p>
* A simple general purpose loggeable message.
Expand Down Expand Up @@ -90,7 +90,7 @@ public void dump(PrintStream p, String indent) {
if (msgContent instanceof Object[])
cl = Arrays.asList((Object[]) msgContent);
else if (msgContent instanceof Collection)
cl = (Collection) msgContent;
cl = new ArrayList<>((Collection) msgContent);
else if (msgContent instanceof Loggeable)
cl = Arrays.asList(msgContent);
else if (msgContent instanceof Throwable)
Expand Down
Loading