RFR: 8329728: Read long lines in ClassListParser [v3]

[David Holmes]

On Wed, 10 Apr 2024 04:02:52 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> Today the `ClassListParser` has a hard-coded limit of 4096 chars for each line in the CDS class list file. However, it's possible for a line to be much longer than than (64KB for the class name, plus extra information that can include path names, IDs, etc).
>> 
>> I wrote a utility class `LineReader` that automatically allocates a buffer before calling `fgets()`. Hopefully this can be useful for other cases where we call `fgets()` with a fixed buffer size.
>> 
>> Max line width is limited to 4M to simplify testing (and avoid running into corner cases when we approach INT_MAX).
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Check class name for valid UTF8 encoding

A few suggestions and a few overflow issues that need fixing.

Thanks.

src/hotspot/share/cds/classListParser.cpp line 179:

> 177:         _token = nullptr;
> 178:         _line_len = 0;
> 179:         error("Input line too long"); // will exit JVM

Can you print the line number and/or line length?

src/hotspot/share/cds/classListParser.cpp line 465:

> 463:     err = "class name too long";
> 464:   } else {
> 465:     int len = (int)strlen(class_name);

Suggestion: save the length first so you don't have to calculate it twice.

src/hotspot/share/utilities/lineReader.cpp line 41:

> 39:   _buffer_len = 0;
> 40:   init(file);
> 41: }

Style nit: Shouldn't we be using initializer lists rather than the constructor body for these simple initializations?

src/hotspot/share/utilities/lineReader.cpp line 90:

> 88:     // _buffer_len will stop at MAX_LEN, so we will never be able to read more than
> 89:     // MAX_LEN chars for a single input line.
> 90:     assert(line_len >= 0 && new_len >= 0 && (line_len + new_len) >= 0, "no int overflow");

The overflow test is relying on UB you need to check subtraction from INT_MAX

src/hotspot/share/utilities/lineReader.cpp line 110:

> 108:         return _buffer;
> 109:       }
> 110:       int new_len = _buffer_len * 2;

Again UB on the overflow check. MAX_LEN should be set so that doubling of the current size will always hit max_len so you can't overflow.

src/hotspot/share/utilities/lineReader.hpp line 36:

> 34: // MAX_LEN is currently 4M. This should be enough for any practical use
> 35: // of text-based input files for HotSpot. Don't use LineReader if it's
> 36: // possible for valid lines to be longer than this limit.

Should be easy enough to make this configurable if needed in the future.

src/hotspot/share/utilities/lineReader.hpp line 37:

> 35: // of text-based input files for HotSpot. Don't use LineReader if it's
> 36: // possible for valid lines to be longer than this limit.
> 37: class LineReader : public StackObj {

Suggestion: LineReader should also track the line count.

src/hotspot/share/utilities/lineReader.hpp line 61:

> 59:   // When successful, a non-null value is returned. The caller is free to read or modify this
> 60:   // string (up to the terminating \0 character) until the next call to read_line(), or until the
> 61:   // LineReader is destructed.

s/destructed/destroyed/

-------------

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18669#pullrequestreview-1990789505
PR Review Comment: https://git.openjdk.org/jdk/pull/18669#discussion_r1558838077
PR Review Comment: https://git.openjdk.org/jdk/pull/18669#discussion_r1558839884
PR Review Comment: https://git.openjdk.org/jdk/pull/18669#discussion_r1558841905
PR Review Comment: https://git.openjdk.org/jdk/pull/18669#discussion_r1558849350
PR Review Comment: https://git.openjdk.org/jdk/pull/18669#discussion_r1558851648
PR Review Comment: https://git.openjdk.org/jdk/pull/18669#discussion_r1558853079
PR Review Comment: https://git.openjdk.org/jdk/pull/18669#discussion_r1558853376
PR Review Comment: https://git.openjdk.org/jdk/pull/18669#discussion_r1558853942