> On Apr 8, 2019, at 9:38 PM, Sean Mullan <sean.mul...@oracle.com> wrote:
>
> On 4/6/19 10:33 AM, Weijun Wang wrote:
>> 603 // check that the Class of the Permission key and value are the
>> same
>> 604 for (Map.Entry<Permission, Permission> e : perms.entrySet()) {
>> 605 Permission k = e.getKey();
>> 606 Permission v = e.getValue();
>> 607 if (!(k.getClass().equals(v.getClass()))) {
>> 608 throw new InvalidObjectException("Permission with " +
>> 609 k.getClass() + " incorrectly mapped to Permission
>> with " +
>> 610 v.getClass());
>> 611 }
>> 612 }
>> Why not check k == v? If they were the same object at serialization, then
>> the deserialization process should be able to assign the same object to them.
>
> I agree, good catch. I have changed the above block of code to:
>
> 603 // check that the Permission key and value are the same object
> 604 for (Map.Entry<Permission, Permission> e : perms.entrySet()) {
> 605 Permission k = e.getKey();
> 606 Permission v = e.getValue();
> 607 if (k != v) {
> 608 throw new InvalidObjectException("Permission (" + k +
> 609 ") incorrectly mapped to Permission (" + v + ")");
> 610 }
> 611 }
>
> If you are ok with that, I will push the fix.
I have no other comment. Please go on.
Thanks,
Max
>
> --Sean
>
>> --Max
>>> On Apr 6, 2019, at 1:30 AM, Sean Mullan <sean.mul...@oracle.com> wrote:
>>>
>>> Updated webrev:
>>> http://cr.openjdk.java.net/~mullan/webrevs/8020637/webrev.01/
>>>
>>> The serialized streams are now encoded within the test code itself. I also
>>> added a test case for an PermissionsHash object with invalid mappings.
>>>
>>> I also modified the fix. Instead of trying to fix the mappings in the
>>> encoded serialized form, readObject simply throws an
>>> InvalidObjectException. I believe that is the more correct behavior, as the
>>> deserialized form no longer adheres to the invariants of the serialized
>>> structures of the corresponding classes.
>>>
>>> Thanks,
>>> Sean
>>>
>>> On 4/2/19 10:03 AM, Weijun Wang wrote:
>>>> +1.
>>>> --Max
>>>>> On Apr 2, 2019, at 9:55 PM, Roger Riggs <roger.ri...@oracle.com> wrote:
>>>>>
>>>>> Hi Sean,
>>>>>
>>>>> Typically, fixed serialization streams are encoded in the source
>>>>> as byte arrays. That keeps binary content out of the repo
>>>>> and provides a place for the comments.
>>>>>
>>>>> Roger
>>>>>
>>>>>
>>>>> On 04/02/2019 09:50 AM, Sean Mullan wrote:
>>>>>> On 4/2/19 9:44 AM, Weijun Wang wrote:
>>>>>>>
>>>>>>>
>>>>>>>> On Apr 2, 2019, at 9:33 PM, Sean Mullan <sean.mul...@oracle.com> wrote:
>>>>>>>>
>>>>>>>> On 4/1/19 11:12 PM, Weijun Wang wrote:
>>>>>>>>> I can understand the change in Permissions, but is there any
>>>>>>>>> difference in PermissionsHash?
>>>>>>>>
>>>>>>>> The key and value in the PermissionsHash map is always the same
>>>>>>>> object. This fix ensures that is respected, otherwise after
>>>>>>>> deserialization you could have a SocketPermission mapped to a
>>>>>>>> FilePermission, for example. Would it be better if I added a test for
>>>>>>>> that?
>>>>>>>
>>>>>>> Yes, you are right. I thought the old code can also enforce this
>>>>>>> relation.
>>>>>>>
>>>>>>> Now for the test, perms.ser is binary and you haven't described how it
>>>>>>> is generated.
>>>>>>
>>>>>> I just hacked Permissions.writeObject to switch the mappings. That was a
>>>>>> lot easier than trying to write my own serialization code. I will add
>>>>>> some comments in the test explaining how I did that and what is in
>>>>>> perms.ser.
>>>>>>
>>>>>> --Sean
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Max
>>>>>>>
>>>>>>>>
>>>>>>>> --Sean
>>>>>>>>
>>>>>>>>> --Max
>>>>>>>>>> On Apr 2, 2019, at 1:10 AM, Sean Mullan <sean.mul...@oracle.com>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> It is currently possible to change the mappings in a serialized
>>>>>>>>>> java.security.Permissions object such that they no longer map
>>>>>>>>>> correctly, and Permissions.readObject won't detect this.
>>>>>>>>>>
>>>>>>>>>> This change makes sure that for a deserialized Permissions object,
>>>>>>>>>> the permissions are mapped correctly to the class that they belong
>>>>>>>>>> to. It does this by calling add() again for each permission in the
>>>>>>>>>> deserialized Permissions object. The same technique was applied to a
>>>>>>>>>> serialized PermissionsHash object which is used to store Permissions
>>>>>>>>>> that don't implement their own PermissionCollection.
>>>>>>>>>>
>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8020637
>>>>>>>>>> webrev: http://cr.openjdk.java.net/~mullan/webrevs/8020637/webrev.00/
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Sean
>>>>>>>>>>
>>>>>>>
>>>>>
