[GitHub] [cordova-android] Hanzofm edited a comment on issue #740: Problem security audit and code correctness

[GitBox <gi...@apache.org>]

Hanzofm edited a comment on issue #740: Problem security audit and code correctness
URL: https://github.com/apache/cordova-android/issues/740#issuecomment-500349581
 
 
   After read some documentation of "Double check" problem I would like to share my conclusions and see if it if it would be fixed on the platform.
   
   Related with the [Wiki](https://en.wikipedia.org/wiki/Double-checked_locking) page of the known code mistake, in the Java section this is a similar example of a reported code by audit:
   
   ```
   class Foo {
       private Helper helper;
       public Helper getHelper() {
           if (helper == null) {
               synchronized (this) {
                   if (helper == null) {
                       helper = new Helper();
                   }
               }
           }
           return helper;
       }
   
       // other functions and members...
   }
   ```
   The problem is about concurrency in extremely cases copy-paste:
   
   For example, consider the following sequence of events:
   
   > 1.Thread A notices that the value is not initialized, so it obtains the lock and begins to initialize the value.
   > 2.Due to the semantics of some programming languages, the code generated by the compiler is allowed to update the shared variable to point to a partially constructed object before A has finished performing the initialization. For example, in Java if a call to a constructor has been inlined then the shared variable may immediately be updated once the storage has been allocated but before the inlined constructor initializes the object.[6]
   > 3.Thread B notices that the shared variable has been initialized (or so it appears), and returns its value. Because thread B believes the value is already initialized, it does not acquire the lock. If B uses the object before all of the initialization done by A is seen by B (either because A has not finished initializing it or because some of the initialized values in the object have not yet percolated to the memory B uses (cache coherence)), the program will likely crash.
   
   
   In relation to your code it would be similar with:
   
   ```
   public class NativeToJsMessageQueue {
   ...
   private BridgeMode activeBridgeMode; // helper in the example
   
   ...
   
    public void setBridgeMode(int value) {
           if (value < -1 || value >= bridgeModes.size()) {
               LOG.d(LOG_TAG, "Invalid NativeToJsBridgeMode: " + value);
           } else {
               BridgeMode newMode = value < 0 ? null : bridgeModes.get(value);
               if (newMode != activeBridgeMode) {
                   LOG.d(LOG_TAG, "Set native->JS mode to " + (newMode == null ? "null" : newMode.getClass().getSimpleName()));
                   synchronized (this) {
                       activeBridgeMode = newMode; // inline asignation
                       if (newMode != null) { // double check problem
                           newMode.reset();
                           if (!paused && !queue.isEmpty()) {
                               newMode.onNativeToJsMessageAvailable(this);
                           }
                       }
                   }
               }
           }
       } 
   
   ...
   }
   ```
   According to the Wiki article the problem can be avoided on Java versions >1.5 with the `volatile` keyword like this example:
   
   ```
   class Foo {
       private volatile Helper helper;
       public Helper getHelper() {
           Helper localRef = helper;
           if (localRef == null) {
               synchronized (this) {
                   localRef = helper;
                   if (localRef == null) {
                       helper = localRef = new Helper();
                   }
               }
           }
           return localRef;
       }
   
       // other functions and members...
   }
   ```
   
   > Note the local variable "localRef", which seems unnecessary. The effect of this is that in cases where helper is already initialized (i.e., most of the time), the volatile field is only accessed once (due to "return localRef;" instead of "return helper;"), which can improve the method's overall performance by as much as 25 percent
   
   Do you think it would be possible for you to implement it in the repository?
   
   Thanks a lot

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org