RFR: 8275846: read_base_archive_name() could read past the end of buffer

[Thomas Stuefe]

On Fri, 5 Nov 2021 20:11:10 GMT, Ioi Lam <iklam at openjdk.org> wrote:

> Please review this small fix for an intermittent crash in `FileHeaderHelper::read_base_archive_name()`. The crash happens if a string stored inside a CDS dynamic archive is not zero-terminated.
> 
> The fix is to check for zero-termination, and that the actual string length matches with the the recorded length.
> 
> This fix was authored by @yqi in PR #5991 , but that PR may take longer to integrate than expected. So let's fix the crash first.

Looks good. Just questions and idle bikeshedding.

..Thomas

src/hotspot/share/cds/filemap.cpp line 1086:

> 1084:   }
> 1085: 
> 1086:   char* read_base_archive_name() {

Why did you change the prototype? Seemed alright to me before, but may be a matter of taste.

src/hotspot/share/cds/filemap.cpp line 1088:

> 1086:   char* read_base_archive_name() {
> 1087:     assert(_fd != -1, "Archive should be open");
> 1088:     size_t name_size = _header._base_archive_name_size;

Unrelated, but do we do sanity checking of header data? E.g. for a max reasonable length of _base_archive_name_size?

src/hotspot/share/cds/filemap.cpp line 1098:

> 1096:       return nullptr;
> 1097:     }
> 1098:     if (base_name[name_size - 1] != '\0' || strlen(base_name) != name_size - 1) {

First condition to prevent strlen overruns? Second to check if name in header was somehow zero'd out?

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

Marked as reviewed by stuefe (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6285