RFR: 8328995: launcher can't open jar files where the offset of the manifest is >4GB [v4]

[Jiangli Zhou]

On Fri, 29 Mar 2024 18:43:51 GMT, Liam Miller-Cushon <cushon at openjdk.org> wrote:

>> src/java.base/share/native/libjli/parse_manifest.c line 506:
>> 
>>> 504:               jlong offset = 0;
>>> 505:               while (offset < cenext) {
>>> 506:                 short headerId = ZIPEXT_HDR(base + offset);
>> 
>> This block of code is very similar to the corresponding part (reading the ZIP64 extension extra fields) in `validate_lochdr`. Perhaps also refractor to a common helper function?
>
> The other loop uses `readAt` to read in additional data and advance through the extra fields, this loop already has access to a buffer that contains all of the data for the extra fields and doesn't need to do that.
> 
> I considered having the other loop read in all of the data for the extra fields so there could be a shared helper, but the data is variable length, so that would have required having a large fixed-size buffer or allocating memory, which this code seems to be trying to avoid.
> 
> Do you see a good way to share the loop?

How about something like the following (I haven't tried to compile or test, please double check if everything is correct)? The size of the extra fields is recorded in the 2-byte field, `extra field length`, so we can just read all extra fields in a temp buffer. It causes less overhead with just one read. 


jboolean read_zip64_ext(int fd, jlong censtart, jlong base_offset, Byte *cenhdr,
                                           jboolean check_offset_only, jboolean read_extra_fields) {
  jlong cenlen = CENLEN(cenhdr);
  jlong censiz = CENSIZ(cenhdr);
  jlong cenoff = CENOFF(cenhdr);
  if (cenoff == ZIP64_MAGICVAL ||
      (!check_offset_only && (censiz == ZIP64_MAGICVAL || cenlen == ZIP64_MAGICVAL))) {
    jlong cenext = CENEXT(cenhdr);
    if (cenext == 0) {
      return JNI_FALSE;
    }

    jlong start = censtart + CENHDR + CENNAM(cenhdr);
    jlong offset = 0;
    Byte *p = start;

    // Read the extra fields if needed.
    if (read_extra_fields) {
      *p = (Byte*)malloc(cenext);
      if (p == NULL) {
        return JNI_FALSE;
      }
      if (!readAt(fd, start + offset, cenext, p)) {
        free(p);
        return JNI_FALSE;
      } 
    }
    
    while (offset < cenext) { 
      short headerId = ZIPEXT_HDR(p + offset);
      short headerSize = ZIPEXT_SIZ(p + offset);
      if (headerId == ZIP64_EXTID) {
        if (!read_zip64_ext(p, &cenlen, &censiz, &cenoff, CENDSK(cenhdr))) {
          if (p != start) {
            free(p);
          }
          return JNI_FALSE;
        }
        break;
      }
      offset += 4 + headerSize;
    }
  }
  
  if (p != start) {
    free(p);
  }
  return JNI_TRUE;
}

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1545055704