RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v3]

[Jaikiran Pai]

Hello Alan,

On 04/07/21 8:47 pm, Alan Bateman wrote:
> On 03/07/2021 17:19, Jaikiran Pai wrote:
>> :
>>
>>
>> There are still some decisions to be made:
>>
>> 1. Should we introduce this new property or should we enhance the 
>> existing "useTempFile" property to allow a size to be passed?
> It's an implementation tuning knob and highly unlikely to be in the 
> environment specified to newFileSystem. So I think the simplest is to 
> just leave it out, it can always be added at a later time if there is 
> a new real need. The default that you've chosen (10MB) looks a 
> reasonable value to start with.
>
> BTW: The documented properties that can specified in the environment 
> are in jdk.zipfs's module description. The useTempFile property is not 
> listed here as it was never a documented/supported property that dates 
> back to early iterations of the zip file system when it was a demo.

I've updated the PR to make this an internal implementation detail/value 
and not expect it to be a configuration passed during the creation of 
the file system. This also allowed me to simplify some of this new code 
to no longer check for 0 or negative values for this property and other 
related semantics.


>
> As the PR has now expanded to do JDK-8011146 (and it doesn't matter if 
> you use that JBS issue or continue with JDK-8190753) then I think the 
> priority should be to be confident with the security.
>
Couple of things that I can think of when it comes to security is making 
sure the temp file is created in the right location and right set of 
permissions and making sure that the temp file isn't left behind after 
the filesystem is closed or after the zip entry is no longer relevant. 
When it comes to temp file creation, the code in this PR uses existing 
constructs/code that's present in this jdk.nio.zipfs.ZipFileSystem 
class. I will take a detailed look at this part once I get to my 
workstation later today. Are there any other security aspects that we 
need to think about?


>> 4. I've never previously created a manual test case. The 
>> `LargeCompressedEntrySizeTest` in this PR is expected to be a manual 
>> test case (given how long it might take to run on various different 
>> systems). The only difference between this test case and other jtreg 
>> automated tests is the absence of a `@test` on this one. Is this how 
>> manual tests are written or is there some other way?
>>
> We avoid manual tests as there is no guarantee that they will be run. 
> So maybe we'll need to explore the scenario a bit further to see if 
> there is some way to come up with an automated test. The jtreg foo for 
> manual tests is `@run main/manual LargeCompressedEntrySizeTest`. 
> You'll see a few examples in the test suite but I don't know if they 
> are ever run.

I have updated the PR to use jtreg's construct of @run testng/manual to 
mark this as a manual test. I will post the timing of this test case 
later today after I run the latest version locally and see how long it's 
taking.

-Jaikiran