Data Integrity Corruption in AbstractPrimitive.putAll

[marcello@…]

The AbstractPrimitive.putAll function does not copy-on-write the keys when only existing values are changed.

How can this lead to data corruption? If you clone a primitive and change a value on the clone, the value will also change on the original because they share the keys array.

[taylor.smock]

This is actually a good point. We probably shouldn't use the same String[] array for the keys (defensive copy).

I don't know how this would lead to data corruption in normal use -- I'd have to look through the code a bit, but there are a few commands that clone stuff.

Did you run into a problem somewhere, and if so, would doing a defensive copy in putAll fix it? (i.e., String[] newKeys = keys.clone())

Putting the .clone in setKeys(TagMap) might be better though.

[marcello@…]

It says in AbstractPrimitive.java that the keys are synchronized by RCU. It also says in putAll: "Defensive copy of keys". It is therefore surprising behaviour that in some cases putAll does not copy the keys. This breaks the RCU contract.

Currently there is only one use of putAll in VectorDataStore.java.

This had me debugging new code for the better part of a day until I found that the error wasn't mine.

The bug is that putAll only makes a copy if tags where added if (!tagsToAdd.isEmpty()) { but it should make a copy on any kind of change.

[taylor.smock]

In 18617/josm:

Fix #22580: AbstractPrimitive#putAll was not creating a copy of the keys array

This broke the RCU contract (Read-Copy-Update), since we were not modifying a
copy of the array.

[taylor.smock]

The original defensive copy was to avoid having the key field change unexpectedly.

Anyway, thanks for opening this ticket.