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

[Ioi Lam]

On Wed, 10 Apr 2024 04:50:15 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Check class name for valid UTF8 encoding
>
> 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?

The line number is printed inside `error()`.

Also, I changed the error message to "Out of memory", as we'd come here only if LineReader fails to expand its internal buffer. If the line is too long, it will be broken into multiple chunks. See the gtest case.

> 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.

Done.

> 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?

Done

> 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

Fixed

> 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.

Fixed

> 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.

Done. As a result, I removed `ClassListParser::_line_no`.

> 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/

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18669#discussion_r1559845162
PR Review Comment: https://git.openjdk.org/jdk/pull/18669#discussion_r1559845081
PR Review Comment: https://git.openjdk.org/jdk/pull/18669#discussion_r1559845012
PR Review Comment: https://git.openjdk.org/jdk/pull/18669#discussion_r1559844933
PR Review Comment: https://git.openjdk.org/jdk/pull/18669#discussion_r1559844873
PR Review Comment: https://git.openjdk.org/jdk/pull/18669#discussion_r1559844698
PR Review Comment: https://git.openjdk.org/jdk/pull/18669#discussion_r1559844670