> On Aug 27, 2020, at 10:36 AM, Seán Coffey <sean.cof...@oracle.com> wrote:
>
> Thanks for the review Max. Comments inline..
>
> On 27/08/2020 14:45, Weijun Wang wrote:
>> I’m OK with using one warning, but prefer it to a little more formal like
>> "POSIX file permission and/or symlink attributes detected…”.
>>
>> One nit in ZipFile.java:
>>
>> 1098 // only set posix perms value via ZipEntry constructor
>> for now
>> 1099 @Override
>> 1100 public int getExtraAttributes(ZipEntry ze) {
>>
>> Maybe you can just remove the comment.
>>
>> Do you also want to rename the “posixPermsDetected" field and loacl variable
>> “perms” in JarSigner.java?
>
> Good points. Edits made.
>
> http://cr.openjdk.java.net/~coffeys/webrev.8250968.v3/webrev/
>
>>
>> I’m not sure about the test but if zipfs is able to keep permissions inside
>> a zip file then that POSIX byte (or whatever it’s named) is already there
>> and we can modify it to include more bits.
>
> Looks like it was a conscious design decision to only allow recording of
> POSIX permission bits for this field (& 0xFFF). I don't see anything about
> symlink support in zipfs docs.
As long as that *byte* is there and it’s not difficult to locate, we can
manually add the *bit* for symlink and see if jarsigner can keep it.
—Max
>
> regards,
> Sean.
>
>>
>> No other comment.
>>
>> Thanks,
>> Max
>>
>>
>>> On Aug 27, 2020, at 3:26 AM, Seán Coffey <sean.cof...@oracle.com> wrote:
>>>
>>> updated webrev:
>>> http://cr.openjdk.java.net/~coffeys/webrev.8250968.v2/webrev/
>>>
>>> regards,
>>> Sean.
>>>
>>> On 27/08/2020 07:42, Seán Coffey wrote:
>>>> Hi Max,
>>>>
>>>> I looked at updating the warning string but figured that it might have
>>>> been of no interest to end users. How about this edit then ?
>>>>
>>>> + {"posix.attributes.detected", "POSIX file permission attributes
>>>> detected. These attributes are ignored when signing and are not protected
>>>> by the signature."},
>>>>
>>>>>> replace with:
>>>> + {"extra.attributes.detected", "POSIX file permission/symlink
>>>> attributes detected. These attributes are ignored when signing and are not
>>>> protected by the signature."},
>>>>
>>>> regards,
>>>> Sean.
>>>>
>>>> On 26/08/2020 23:15, Weijun Wang wrote:
>>>>> Are you going to update the warning text or create a new one?
>>>>>
>>>>> Thanks,
>>>>> Max
>>>>>
>>>>>> On Aug 26, 2020, at 2:26 PM, Seán Coffey <sean.cof...@oracle.com> wrote:
>>>>>>
>>>>>> This is a follow on from the recent 8218021 fix. The jarsigner tool
>>>>>> removes symlink attribute data from zipfiles when signing them.
>>>>>> jarsigner should preserve this data. The fix involves preserving the 16
>>>>>> bits associated with the file attributes (instead of the current 12).
>>>>>> That's done in ZipFile. All other changes are just a refactor of the
>>>>>> variable name.
>>>>>>
>>>>>> I haven't been able to automate a test for this since zipfs doesn't seem
>>>>>> to support symlinks. Manual testing looks good.
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8250968
>>>>>> http://hmsjpse.uk.oracle.com/home/scoffey/ws/jdk-jdk/open/webrev.8250968/webrev/index.html
>>>>>>
>>>>>> regards,
>>>>>> Sean.
>>>>>>
