android / codelab-android-room-with-a-view

Apache License 2.0
746 stars 490 forks source link

Double-Checked Locking reported as anti-pattern CWE-609 #218

Open stephankn opened 2 years ago

stephankn commented 2 years ago

The getDatabase() code tries to avoid explicit synchronization by applying a double-ckecked locking pattern.

https://developer.android.com/codelabs/android-room-with-a-view#7

public abstract class WordRoomDatabase extends RoomDatabase {

   private static volatile WordRoomDatabase INSTANCE;

   static WordRoomDatabase getDatabase(final Context context) {
        if (INSTANCE == null) {
            synchronized (WordRoomDatabase.class) {
                if (INSTANCE == null) {
                    INSTANCE = Room.databaseBuilder(context.getApplicationContext(),
                            WordRoomDatabase.class, "word_database")
                            .build();
                }
            }
        }
        return INSTANCE;
    }
}

The way it is implemented here looks like as a broken implementation as per CWE-609. https://cwe.mitre.org/data/definitions/609.html

I also get warnings by deepsource about the same, stating:

The JVM is free to change the order of operations with respect to how objects are created and assigned to references. https://deepsource.io/directory/analyzers/java/issues/JAVA-E1018

The code is slightly different from the CWE sample, as there an assignment of a newly constructed object happens. But somewhere within the Builder also a new object is instantiated. Is there a guarantee that the construction has completed before the assignment happens? If not, the code sample should use one of the better approaches as outlined by deepsource.