Hi Sean,
I think the changes are OK in the latest version. A couple of the files have a
2019 copyright still.
> On Aug 27, 2020, at 10:58 AM, Weijun Wang <weijun.w...@oracle.com> wrote:
>
>
>
>> 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.
We can create an RFE for adding support for this with Zip FS.
>
> —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.
Best
Lance
------------------
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
