Skip to content

Structured logging: a failed JSON encode corrupts the next log event written on the same thread - #51156

Merged
snicoll merged 2 commits into
spring-projects:4.1.xfrom
skdas20:fix-51154-appendable-byte-array-reset
Aug 11, 2026
Merged

snicoll merged 2 commits into
spring-projects:4.1.xfrom
skdas20:fix-51154-appendable-byte-array-reset

Conversation

@skdas20

@skdas20 skdas20 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

See gh-51154.

AppendableByteArray.reset() is only reached from toByteArray():

byte[] result = new byte[size];
System.arraycopy(...);
reset();          // only on the success path
return result;

so when to(appendable) in WritableJson.toByteArray(Charset) throws part-way through, the partially written bytes stay in the instance held by the ThreadLocal cache, and get() hands that dirty instance to the next caller on the same thread. In structured logging that means one failed event corrupts the next event into invalid JSON, as reported.

This resets the cached instance when it is handed out rather than when it is drained, so the buffer is clean regardless of how the previous use ended. That also covers the early return in toByteArray():

if (size <= 0) {
    return NO_BYTES;   // returns without reset(), leaving the buffer flipped
}

which the alternative (resetting on the failure path in WritableJson) would not.

Tests added:

  • AppendableByteArrayTests.getWhenPreviousUseWasAbandonedReturnsCleanInstance — covers the cache contract directly: append to an instance, never drain it, then assert the next get() returns it clean.
  • WritableJsonTests.toByteArrayWhenPreviousWriteFailedDoesNotIncludePartialContent — the reported end-to-end shape: a failed encode followed by an unrelated writer on the same thread.

Both fail on main and pass with the change. org.springframework.boot.json.*, the logging.structured tests, and checkFormatMain/checkFormatTest are all green locally.

I raised the analysis on the issue first and offered either placement — happy to move the reset into WritableJson.toByteArray instead if you'd prefer to keep get() free of side effects, or to close this if you'd rather fix it in-house given it's a regression in recent code.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 29, 2026
@snicoll snicoll self-assigned this Aug 11, 2026
@snicoll snicoll added type: regression A regression from a previous release and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 11, 2026
@snicoll snicoll added this to the 4.1.1 milestone Aug 11, 2026
@snicoll snicoll changed the title Reset cached AppendableByteArray before it is reused Structured logging: a failed JSON encode corrupts the next log event written on the same thread Aug 11, 2026
@snicoll
snicoll changed the base branch from main to 4.1.x August 11, 2026 15:15
skdas20 and others added 2 commits August 11, 2026 17:59
AppendableByteArray.reset() was only called from toByteArray(), so an
encode that failed part-way left its partial output in the thread-local
cached instance. The next value encoded on that thread was then prefixed
with it, which in structured logging corrupted the log event following a
failed one.

Reset the cached instance when it is handed out instead, so the buffer
is clean regardless of how the previous use ended. This also covers the
early return in toByteArray() for empty content, which returns without
resetting.

See spring-projectsgh-51156

Signed-off-by: Sumit Kumar Das <skdas5405@gmail.com>
@snicoll
snicoll force-pushed the fix-51154-appendable-byte-array-reset branch from dfc9f7a to dafadea Compare August 11, 2026 15:59
@snicoll
snicoll merged commit af69828 into spring-projects:4.1.x Aug 11, 2026
1 check failed
@snicoll

snicoll commented Aug 11, 2026

Copy link
Copy Markdown
Member

@skdas20 thanks for the PR but I decided to go in a different direction. I've reworked the PR, see the polish commit for more details.

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

Labels

type: regression A regression from a previous release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants