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

[Ioi Lam]

On Sat, 6 Nov 2021 05:54:49 GMT, Thomas Stuefe <stuefe 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 #5997 , but that PR may take longer to integrate than expected. So let's fix the crash first.
>
> 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?

I am planning to do that in a follow-on RFE (https://bugs.openjdk.java.net/browse/JDK-8276769). I will add more structural checks:

- GenericCDSFileMapHeader::_header_size must be smaller than the archive file
- _base_archive_name_size/_base_archive_path_offset must be inside the header
- the header must not overlap with the regions
- the regions should not overlap with each other

Since more checks are added, I need to make sure the new checks also respect the -Xshare:auto flag (failure should not cause the VM to exit) which is the purpose of JDK-8276769.

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

Yes.

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

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