Yes, I also agree on on the defensive copy. On Mon, Dec 15, 2008 at 11:36 AM, Tony Wu <[email protected]> wrote:
> On Mon, Dec 15, 2008 at 2:18 AM, Alexey Varlamov > <[email protected]> wrote: > > 2008/12/13 Kevin Zhou <[email protected]>: > >> Alexey wrote, > >>> One must make defensive copy when setting array properties - plain > >> assigment of array reference is unsafe. > >> Yes, this assigment of arry reference is unsafe. But no spec says that > the > >> array of certificates in a UnresovledPermission need to be thread-safe. > >> If we add a lock to guarantee its safe, how about the other methods > which > >> access to targetCerts array. Do we need to make them safe too? > > > > I did not mean thread safety here, rather internal object integrity. > > BTW, API spec directly says: "The signer certificates are copied from > > the array. Subsequent changes to the array will not affect this > > UnsolvedPermission." > > The same precautions are already in place where needed. > > > > Alexey, I agree with you on the defensive copy. > > >>>Looking on the test source, I'd rather consider this as non-bug > difference. > >> > >> I think this is different behaviors between RI and HARMONY of > determining > >> certificate equality? > >> The previous implementation of HARMONY uses PolicyUtils.matchSubSet > method > >> which behaves differently from RI. > >> > >> I think we'd better follow RI's behaviors on this. > > This is better only if RI's behaviour is logical and consistent or > > became widely adapted feature. This is not the case here. E.g., > > throwing NPE on equals() invocation is always considered bad practice, > > etc. Moreover, I wonder if RI is compliant with spec which says: "To > > determine certificate equality, this method only compares actual > > signer certificates. " > > So I suggest to rollback the patch and mark the issue "non-bug > difference". > > > > I think there is a behavior difference if we leave this gap in our > code. The difference is how do we do when encountering a null > certificate. RI takes it into consideration and Harmony just ignore > it. this difference itself is trival but will result in the different > behavior in security. For example, following two instances of > UnresolvedPermission are considered as equal with Harmony but just not > with RI. > > new UnresolvedPermission(type, name, action, new > java.security.cert.Certificate[]{cert1, null}) > new UnresolvedPermission(type, name, action, new > java.security.cert.Certificate[]{cert1}) > > I can not tell what's the impact to any product exactly at this time > but I'd like to mitigate the risk of creating a security hole. > > > -- > > Alexey > > > > > > -- > Tony Wu > China Software Development Lab, IBM >
