While evaluating JDK-8361748 for 11u and 8u backports, I found some problems with the logic of the original change.


Main problem: multi-line bitmap arrays
=============================

All the bitmap bytes are parsed only if the line contains "[]":
https://github.com/openjdk/jdk/blob/jdk-26+11/src/java.desktop/share/classes/sun/awt/image/XbmImageDecoder.java#L154

So the following data lines are being ignored, which for valid.xbm, means the whole image data is ignored:
https://github.com/openjdk/jdk/blob/jdk-26+11/test/jdk/java/awt/image/XBMDecoder/valid.xbm#L6
https://github.com/openjdk/jdk/blob/jdk-26+11/test/jdk/java/awt/image/XBMDecoder/valid_hex.xbm#L4

Unfortunately, the test case isn't checking the parsed image data, so it isn't catching the issue. I suggest at least checking the image data isn't full of zeros (this would detect the problem with valid.xbm), see XBMDecoderTest.java changes in https://bugs.openjdk.org/secure/attachment/118016/JDK-8373727-test-suggestions.patch.

Please note that matching "[]" might also be too restrictive, as whitespace should probably be allowed between the square brackets. But this is a minor issue, and perhaps acceptable for a minimalist parser.


Additional problem: regexes
=====================

I've also spotted some regex mistakes where the intention was to define a group (should be enclosed between parenthesis), but the code defines a character set (is enclosed between square brackets):
 • matchRegex = "(0[xX])?[0-9a-fA-F]+[\\s+]?[,|};]"
   • Very likely intended to be matchRegex = "(0[xX])?[0-9a-fA-F]+(\\s+)?(,|\\};)"
   • And can be simplified as matchRegex = "(0[xX])?[0-9a-fA-F]+\\s*(,|\\};)"
 • replaceRegex = "(0[xX])|,|[\\s+]|[};]"
   • Very likely intended to be replaceRegex = "(0[xX])|,|(\\s+)|(\\};)"
   • And can be simplified as replaceRegex = "0[xX]|,|\\s+|\\};"

As a consequence, invalid files containing a plus sign are not rejected by the parser. I suggest adding a test case for this, see invalid_plus.xbm in https://bugs.openjdk.org/secure/attachment/118016/JDK-8373727-test-suggestions.patch.


New findings: order of width and height definitions
======================================

As we can see in the ImageMagick parser, they don't tolerate any order for the width and height #defines, but they check the suffixes. However the new JDK-8361748 parser ignores the suffixes and expects they to be ordered (first width then height):
https://github.com/ImageMagick/ImageMagick/blob/7.1.1-47/coders/xbm.c#L233-L255
https://github.com/openjdk/jdk/blob/jdk-26+11/src/java.desktop/share/classes/sun/awt/image/XbmImageDecoder.java#L113-L119

The old parser was far from readable and robust, but it loosely distinguished width from height, by matching the last one/two letter(s) in widt<h>/heig<ht>:
https://github.com/openjdk/jdk/blob/jdk-26+10/src/java.desktop/share/classes/sun/awt/image/XbmImageDecoder.java#L109-L112