carterkozak commented on a change in pull request #444:
URL: https://github.com/apache/logging-log4j2/pull/444#discussion_r532609807
##########
File path:
log4j-layout-template-json/src/main/java/org/apache/logging/log4j/layout/template/json/util/JsonWriter.java
##########
@@ -493,18 +492,15 @@ public void writeSeparator() {
final S state) {
Objects.requireNonNull(emitter, "emitter");
stringBuilder.append('"');
- formattableBuffer.setLength(0);
- emitter.accept(formattableBuffer, state);
- final int length = formattableBuffer.length();
- // Handle max. string length complying input.
- if (length <= maxStringLength) {
- quoteString(formattableBuffer, 0, length);
- }
- // Handle max. string length violating input.
- else {
- quoteString(formattableBuffer, 0, maxStringLength);
+ int position = stringBuilder.length();
+ emitter.accept(stringBuilder, state);
+ final int length = stringBuilder.length() - position;
+ if (length > maxStringLength) {
+ // Handle max. string length violating input
+ stringBuilder.setLength(position + maxStringLength);
stringBuilder.append(quotedTruncatedStringSuffix);
}
+ StringBuilders.escapeJson(stringBuilder, position);
Review comment:
> StringBuilders.escapeJson() might add extra characters violating the
max. string length constraint.
Are you confident the implementation on release-2.x and master doesn't have
a bug in this regard? If max-length is 2 and I write the string `a"`, I'd end
up escaping to `a\"`, trimming to `a\`, and appending the truncated string
sequence `\u2026` giving me `a\\u2026` where the backslash is escaped rather
than producing the `…` character. I think we need to take escape sequences into
account when truncating.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
