Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
============

* Your contribution here.
* [#139](https://github.com/jenkinsci/ansicolor-plugin/issues/139)/[#143](https://github.com/jenkinsci/ansicolor-plugin/pull/143): Prevent an `IndexOutOfBoundsException` from being thrown when two or more non-ASCII-compatible characters are present in colored text - [@dwnusbaum](https://github.com/dwnusbaum).

0.6.0 (11/14/2018)
============
Expand Down
30 changes: 17 additions & 13 deletions src/main/java/hudson/plugins/ansicolor/ColorConsoleAnnotator.java
Original file line number Diff line number Diff line change
Expand Up @@ -66,29 +66,33 @@ public ConsoleAnnotator<Object> annotate(Object context, MarkupText text) {
CountingOutputStream outgoing = new CountingOutputStream(new NullOutputStream());
class EmitterImpl implements AnsiAttributeElement.Emitter {
CountingOutputStream incoming;
int adjustment;
int multiByteCharAdjustment;
int hiddenCharAdjustment;
int lastPoint = -1; // multiple HTML tags may be emitted for one control sequence
@Override
public void emitHtml(String html) {
LOGGER.finest("emitting " + html + " @" + incoming.getCount());
text.addMarkup(incoming.getCount(), html);
if (incoming.getCount() != lastPoint) {
lastPoint = incoming.getCount();
int hide = incoming.getCount() - outgoing.getCount() - adjustment;
LOGGER.finest("hiding " + hide + " @" + (outgoing.getCount() + adjustment));
text.addMarkup(outgoing.getCount() + adjustment, outgoing.getCount() + adjustment + hide, "<!--", "-->");
adjustment += hide;
int inCount = incoming.getCount() - multiByteCharAdjustment;
int outCount = outgoing.getCount() - multiByteCharAdjustment + hiddenCharAdjustment;
LOGGER.log(Level.FINEST, "emitting {0} @{1}/{2}", new Object[] { html, inCount, s.length() });
text.addMarkup(inCount, html);
if (inCount != lastPoint) {
lastPoint = inCount;
int hide = inCount - outCount;
LOGGER.log(Level.FINEST, "hiding {0} @{1}", new Object[] { hide, outCount });
text.addMarkup(outCount, outCount + hide, "<!--", "-->");
hiddenCharAdjustment += hide;
}
}
}
EmitterImpl emitter = new EmitterImpl();
CountingOutputStream incoming = new CountingOutputStream(new AnsiHtmlOutputStream(outgoing, colorMap, emitter));
emitter.incoming = incoming;
try {
byte[] data = s.getBytes(charset);
for (int i = 0; i < data.length; i++) {
// Do not use write(byte[]) as offsets in incoming would not be accurate.
incoming.write(data[i]);
for (int i = 0; i < s.length(); i++) {
// We write one UTF-16 code unit at a time so that our counting methods can handle multibyte characters correctly.
byte[] chars = String.valueOf(s.charAt(i)).getBytes(charset);
Copy link
Member Author

@dwnusbaum dwnusbaum Dec 5, 2018

Choose a reason for hiding this comment

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

Thinking about this some more, why do we even use charset here? The text is already a String, and we only use AnsiOutputStream for its calls to emitHtml in the emitter and discard the bytes it writes to the underlying stream. We'd still need to fix this bug no matter what encoding we used here, buy maybe clearer to hard-code UTF-8?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, maybe we need iterate through the Unicode code points rather than the UTF-16 code units in case any characters are not in the BMP? I tried adding emoji to the test case, but it looks like the emoji is converted into the replacement character for unmappable characters somewhere along the line before annotate is called (not sure where), so I don't think it's possible to handle it correctly here (although maybe I was doing something wrong in my test).

Copy link
Member

Choose a reason for hiding this comment

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

I suspect this entire (src/main/) patch could be simplified to

-byte[] data = s.getBytes(charset);
+byte[] data = s.getBytes("US-ASCII");

plus deleting charset and the code that sets it. After all, we do not really care what any of the >U+007F characters in the log are for this purpose—we are throwing out the output and just looking for ESC and stuff, so if we send some (byte) '?' through it will not matter.

(Alternately, do not bother getting a byte[] at all, just loop over chars in s and write one byte for each—either the same, if ASCII, or something arbitrary like '?' otherwise.)

Copy link
Member Author

@dwnusbaum dwnusbaum Dec 11, 2018

Choose a reason for hiding this comment

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

Ok, it seemed easiest to just switch to using US-ASCII so I did that and added an explanatory comment. Edit: Switched to going char-by-char in ffb5045 so that non-BMP characters are handled correctly.

emitter.multiByteCharAdjustment += chars.length - 1;
incoming.write(chars);
}
} catch (IOException x) {
LOGGER.log(Level.WARNING, null, x);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,4 +116,29 @@ public void evaluate() throws Throwable {
}
});
}

@Test
public void testNonAscii() throws Exception {
story.then(r -> {
FreeStyleProject p = r.createFreeStyleProject();
p.getBuildWrappersList().add(new AnsiColorBuildWrapper(null));
p.getBuildersList().add(new TestBuilder() {
@Override
public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
listener.getLogger().println("\033[94;1m[ INFO ] Récupération du numéro de version de l'application\033[0m");
listener.getLogger().println("\033[94;1m[ INFO ] ビルドのコンソール出力を取得します。\033[0m");
return true;
}
});
FreeStyleBuild b = r.buildAndAssertSuccess(p);
StringWriter writer = new StringWriter();
b.getLogText().writeHtmlTo(0L, writer);
String html = writer.toString();
System.out.print(html);
assertThat(html.replaceAll("<!--.+?-->", ""),
allOf(
containsString("<span style=\"color: #4682B4;\"><b>[ INFO ] Récupération du numéro de version de l'application</b></span>"),
containsString("<span style=\"color: #4682B4;\"><b>[ INFO ] ビルドのコンソール出力を取得します。</b></span>")));
});
}
}