Skip to content

Message.put appends every key, so a rewritten key is serialised twice #20

Description

@dmccoystephenson

Problem

Message keeps its keys in an ArrayList alongside the HashMap that holds the values, and
that list is only ever appended to. Two consequences follow, neither of which is guarded against.

A key written twice is emitted twice. Message.put overwrites in the map but appends to the
list unconditionally:

// src/SimpleServer/Message.java:14
public void put(String key, String value) {
    map.put(key, value);
    keys.add(key);
}

toString then iterates the list and looks each key up in the map
(src/SimpleServer/Message.java:30), so a message that had number written to it twice is
serialised with number appearing twice, both occurrences carrying whichever value was written
last.

fromString appends to whatever the message already held. The parser writes its results into
the same two fields rather than clearing them first:

// src/SimpleServer/Message.java:63
map.put(key, value);
keys.add(key);

A Message that is parsed into twice, or that has anything put into it before being parsed into,
therefore accumulates rather than being replaced.

Evidence

Read out of src/SimpleServer/Message.java at d34c1da. No compiler was available in the
session in which this was filed, so no round trip was executed and the consequences described
above are UNVERIFIED by running; they are stated from the control flow of put, toString
and fromString, all three of which are pure string and collection handling with no I/O.

Neither consequence is currently reachable from ClientApp or Protocol, both of which write
each key exactly once into a freshly constructed Message. What is being reported is a latent
trap for the next caller rather than a live defect.

Suggested fix

put could skip the keys.add when the map already contains the key, which preserves first-write
ordering while making a rewrite behave as a replacement. fromString could clear both fields
before parsing. Either change is a few lines and confined to Message.

Note that src/SimpleServer/Message.java is on this loop's do-not-auto-merge list, because both
ends of the protocol depend on it and no CI exists to catch a desync, so a fix here is expected to
be reviewed by hand.


This issue was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

drafted by Claude on behalf of Daniel Stephenson

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions