CODEWITHSUNDEEP
javacollections
Morteza Adi
Morteza Adi

Reputation: 2473

Is this a bug in Java SynchronizedCollection class?

There is a internal class SynchronizedCollection - inside java.util.Collections with two constructor. the first takes the collection and the other takes collection and a mutex. former constructor checks argument for not being null. but the later don't!. here is the implementation.

 SynchronizedCollection(Collection<E> c) {
        if (c==null)
            throw new NullPointerException();
        this.c = c;
        mutex = this;
  }
 SynchronizedCollection(Collection<E> c, Object mutex) {
        this.c = c;
        this.mutex = mutex;
 }

with this implementation i can break the class invariant by sending null to second constructor.

i believe it should be something like this:

 SynchronizedCollection(Collection<E> c) {
        this(c,this)
  }
 SynchronizedCollection(Collection<E> c, Object mutex) {
        if (c==null)
            throw new NullPointerException();
        this.c = c;
        this.mutex = mutex;
 }

however i can't convince myself Josh Bloch and Neal Gafter couldn't see this. so can you really tell me what i missed here?

edited: possible attack

    Map<String, String> m = new Map<String, String>(){

        @Override
        public int size() {
            // TODO Auto-generated method stub
            return 0;
        }

                   .
                   .
                   .

        @Override
        public Collection<String> values() {
            return null;
        }


    };

    Map<String, String> synchronizedMap = Collections.synchronizedMap(m);
    Collection<String> values = synchronizedMap.values();

Upvotes: 9

Views: 657

Answers (2)

mentallurg
mentallurg

Reputation: 5207

Sure this is a bug. The both constructors should be consistent, either both should throw an exception or none should throw.

This has been fixed in Java 8. Now both constructors will throw the exception:

SynchronizedCollection(Collection<E> c) {
    this.c = Objects.requireNonNull(c);
    mutex = this;
}

SynchronizedCollection(Collection<E> c, Object mutex) {
    this.c = Objects.requireNonNull(c);
    this.mutex = Objects.requireNonNull(mutex);
}

Upvotes: 13

Sotirios Delimanolis
Sotirios Delimanolis

Reputation: 280168

Both of those constructors are package protected and only the first one can possibly be used with a null argument through the public synchronizedList() and synchronizedSet() methods of Collections.

The other constructor is used internally (in the Collections class) and the first argument can never be null in the various implementations (calling code) so you cannot break it.

You could always try to create something in the java.util package but you will most likely get a SecurityException.

Upvotes: 3

Related Questions