Fix races during config access, add missing null check (fixes #1194) (#1195)

* Fix race during config reload and missing null check
in FolderListFragment and DeviceListFragment (fixes #1194)

* Review - synchronize(mConfigLock) when mConfig is accessed

* Review - add two breaks in RestApi
in removeFolderInternal, removeDeviceInternal

app/src/main/java/com/nutomic/syncthingandroid/fragments/DeviceListFragment.java

@@ -15,6 +15,7 @@ import com.nutomic.syncthingandroid.activities.DeviceActivity;
 import com.nutomic.syncthingandroid.activities.SyncthingActivity;
 import com.nutomic.syncthingandroid.model.Device;
 import com.nutomic.syncthingandroid.service.Constants;
+import com.nutomic.syncthingandroid.service.RestApi;
 import com.nutomic.syncthingandroid.service.SyncthingService;
 import com.nutomic.syncthingandroid.views.DevicesAdapter;
 
@@ -78,10 +79,17 @@ public class DeviceListFragment extends ListFragment implements SyncthingService
      */
     private void updateList() {
         SyncthingActivity activity = (SyncthingActivity) getActivity();
-        if (activity == null || activity.getApi() == null || !activity.getApi().isConfigLoaded() ||
+        if (activity == null || getView() == null || activity.isFinishing()) {
-                getView() == null || activity.isFinishing())
             return;
-
+        }
+        RestApi restApi = activity.getApi();
+        if (restApi == null || !restApi.isConfigLoaded()) {
+            return;
+        }
+        List<Device> devices = restApi.getDevices(false);
+        if (devices == null) {
+            return;
+        }
         if (mAdapter == null) {
             mAdapter = new DevicesAdapter(activity);
             setListAdapter(mAdapter);
@@ -90,10 +98,9 @@ public class DeviceListFragment extends ListFragment implements SyncthingService
         // Prevent scroll position reset due to list update from clear().
         mAdapter.setNotifyOnChange(false);
         mAdapter.clear();
-        List<Device> devices = activity.getApi().getDevices(false);
         Collections.sort(devices, DEVICES_COMPARATOR);
         mAdapter.addAll(devices);
-        mAdapter.updateConnections(activity.getApi());
+        mAdapter.updateConnections(restApi);
         mAdapter.notifyDataSetChanged();
         setListShown(true);
     }

app/src/main/java/com/nutomic/syncthingandroid/fragments/FolderListFragment.java

@@ -14,6 +14,7 @@ import com.nutomic.syncthingandroid.activities.FolderActivity;
 import com.nutomic.syncthingandroid.activities.SyncthingActivity;
 import com.nutomic.syncthingandroid.model.Folder;
 import com.nutomic.syncthingandroid.service.Constants;
+import com.nutomic.syncthingandroid.service.RestApi;
 import com.nutomic.syncthingandroid.service.SyncthingService;
 import com.nutomic.syncthingandroid.views.FoldersAdapter;
 
@@ -73,10 +74,17 @@ public class FolderListFragment extends ListFragment implements SyncthingService
      */
     private void updateList() {
         SyncthingActivity activity = (SyncthingActivity) getActivity();
-        if (activity == null || activity.getApi() == null || !activity.getApi().isConfigLoaded() ||
+        if (activity == null || getView() == null || activity.isFinishing()) {
-                getView() == null || activity.isFinishing())
             return;
-
+        }
+        RestApi restApi = activity.getApi();
+        if (restApi == null || !restApi.isConfigLoaded()) {
+            return;
+        }
+        List<Folder> folders = restApi.getFolders();
+        if (folders == null) {
+            return;
+        }
         if (mAdapter == null) {
             mAdapter = new FoldersAdapter(activity);
             setListAdapter(mAdapter);
@@ -85,9 +93,8 @@ public class FolderListFragment extends ListFragment implements SyncthingService
         // Prevent scroll position reset due to list update from clear().
         mAdapter.setNotifyOnChange(false);
         mAdapter.clear();
-        List<Folder> folders = activity.getApi().getFolders();
         mAdapter.addAll(folders);
-        mAdapter.updateFolderStatus(activity.getApi());
+        mAdapter.updateFolderStatus(restApi);
         mAdapter.notifyDataSetChanged();
         setListShown(true);
     }

app/src/main/java/com/nutomic/syncthingandroid/service/RestApi.java

@@ -120,6 +120,11 @@ public class RestApi {
      */
     private final Object mAsyncQueryCompleteLock = new Object();
 
+    /**
+     * Object that must be locked upon accessing mConfig
+     */
+    private final Object mConfigLock = new Object();
+
     /**
      * Stores the latest result of {@link #getFolderStatus} for each folder
      */
@@ -199,8 +204,12 @@ public class RestApi {
     }
 
     private void onReloadConfigComplete(String result) {
+        Boolean configParseSuccess;
+        synchronized(mConfigLock) {
             mConfig = new Gson().fromJson(result, Config.class);
-        if (mConfig == null) {
+            configParseSuccess = mConfig != null;
+        }
+        if (!configParseSuccess) {
             throw new RuntimeException("config is null: " + result);
         }
         Log.v(TAG, "onReloadConfigComplete: Successfully parsed configuration.");
@@ -249,12 +258,14 @@ public class RestApi {
      * in {@link EventProcessor#onEvent}.
      */
     public void ignoreDevice(String deviceId) {
+        synchronized (mConfigLock) {
             if (!mConfig.ignoredDevices.contains(deviceId)) {
                 mConfig.ignoredDevices.add(deviceId);
                 sendConfig();
                 Log.d(TAG, "Ignored device [" + deviceId + "]");
             }
         }
+    }
 
     /**
      * Permanently ignore a folder share request.
@@ -262,21 +273,25 @@ public class RestApi {
      * in {@link EventProcessor#onEvent}.
      */
     public void ignoreFolder(String folderId) {
+        synchronized (mConfigLock) {
             if (!mConfig.ignoredFolders.contains(folderId)) {
                 mConfig.ignoredFolders.add(folderId);
                 sendConfig();
                 Log.d(TAG, "Ignored folder [" + folderId + "]");
             }
         }
+    }
 
     /**
      * Undo ignoring devices and folders.
      */
     public void undoIgnoredDevicesAndFolders() {
         Log.d(TAG, "Undo ignoring devices and folders ...");
+        synchronized (mConfigLock) {
             mConfig.ignoredDevices.clear();
             mConfig.ignoredFolders.clear();
         }
+    }
 
     /**
      * Override folder changes. This is the same as hitting
@@ -294,7 +309,11 @@ public class RestApi {
      * EventProcessor will trigger this.reloadConfig().
      */
     private void sendConfig() {
-        new PostConfigRequest(mContext, mUrl, mApiKey, new Gson().toJson(mConfig), null);
+        String jsonConfig;
+        synchronized (mConfigLock) {
+            jsonConfig = new Gson().toJson(mConfig);
+        }
+        new PostConfigRequest(mContext, mUrl, mApiKey, jsonConfig, null);
         mOnConfigChangedListener.onConfigChanged();
     }
 
@@ -302,7 +321,11 @@ public class RestApi {
      * Sends current config and restarts Syncthing.
      */
     public void saveConfigAndRestart() {
-        new PostConfigRequest(mContext, mUrl, mApiKey, new Gson().toJson(mConfig), result -> {
+        String jsonConfig;
+        synchronized (mConfigLock) {
+            jsonConfig = new Gson().toJson(mConfig);
+        }
+        new PostConfigRequest(mContext, mUrl, mApiKey, jsonConfig, result -> {
             Intent intent = new Intent(mContext, SyncthingService.class)
                     .setAction(SyncthingService.ACTION_RESTART);
             mContext.startService(intent);
@@ -322,7 +345,10 @@ public class RestApi {
     }
 
     public List<Folder> getFolders() {
-        List<Folder> folders = deepCopy(mConfig.folders, new TypeToken<List<Folder>>(){}.getType());
+        List<Folder> folders;
+        synchronized (mConfigLock) {
+            folders = deepCopy(mConfig.folders, new TypeToken<List<Folder>>(){}.getType());
+        }
         Collections.sort(folders, FOLDERS_COMPARATOR);
         return folders;
     }
@@ -331,34 +357,43 @@ public class RestApi {
      * This is only used for new folder creation, see {@link FolderActivity}.
      */
     public void createFolder(Folder folder) {
+        synchronized (mConfigLock) {
             // Add the new folder to the model.
             mConfig.folders.add(folder);
             // Send model changes to syncthing, does not require a restart.
             sendConfig();
         }
+    }
 
     public void updateFolder(Folder newFolder) {
+        synchronized (mConfigLock) {
             removeFolderInternal(newFolder.id);
             mConfig.folders.add(newFolder);
             sendConfig();
         }
+    }
 
     public void removeFolder(String id) {
+        synchronized (mConfigLock) {
             removeFolderInternal(id);
             // mCompletion will be updated after the ConfigSaved event.
             sendConfig();
             // Remove saved data from share activity for this folder.
+        }
         PreferenceManager.getDefaultSharedPreferences(mContext).edit()
                 .remove(ShareActivity.PREF_FOLDER_SAVED_SUBDIRECTORY+id)
                 .apply();
     }
 
     private void removeFolderInternal(String id) {
+        synchronized (mConfigLock) {
             Iterator<Folder> it = mConfig.folders.iterator();
             while (it.hasNext()) {
                 Folder f = it.next();
                 if (f.id.equals(id)) {
                     it.remove();
+                    break;
+                }
             }
         }
     }
@@ -369,7 +404,10 @@ public class RestApi {
      * @param includeLocal True if the local device should be included in the result.
      */
     public List<Device> getDevices(boolean includeLocal) {
-        List<Device> devices = deepCopy(mConfig.devices, new TypeToken<List<Device>>(){}.getType());
+        List<Device> devices;
+        synchronized (mConfigLock) {
+            devices = deepCopy(mConfig.devices, new TypeToken<List<Device>>(){}.getType());
+        }
 
         Iterator<Device> it = devices.iterator();
         while (it.hasNext()) {
@@ -397,45 +435,60 @@ public class RestApi {
 
     public void addDevice(Device device, OnResultListener1<String> errorListener) {
         normalizeDeviceId(device.deviceID, normalizedId -> {
+            synchronized (mConfigLock) {
                 mConfig.devices.add(device);
                 sendConfig();
+            }
         }, errorListener);
     }
 
     public void editDevice(Device newDevice) {
+        synchronized (mConfigLock) {
             removeDeviceInternal(newDevice.deviceID);
             mConfig.devices.add(newDevice);
             sendConfig();
         }
+    }
 
     public void removeDevice(String deviceId) {
+        synchronized (mConfigLock) {
             removeDeviceInternal(deviceId);
             // mCompletion will be updated after the ConfigSaved event.
             sendConfig();
         }
+    }
 
     private void removeDeviceInternal(String deviceId) {
+        synchronized (mConfigLock) {
             Iterator<Device> it = mConfig.devices.iterator();
             while (it.hasNext()) {
                 Device d = it.next();
                 if (d.deviceID.equals(deviceId)) {
                     it.remove();
+                    break;
+                }
             }
         }
     }
 
     public Options getOptions() {
+        synchronized (mConfigLock) {
             return deepCopy(mConfig.options, Options.class);
         }
+    }
 
     public Config.Gui getGui() {
+        synchronized (mConfigLock) {
             return deepCopy(mConfig.gui, Config.Gui.class);
         }
+    }
 
     public void editSettings(Config.Gui newGui, Options newOptions) {
+        synchronized (mConfigLock) {
             mConfig.gui = newGui;
             mConfig.options = newOptions;
         }
+    }
 
     /**
      * Returns a deep copy of object.
@@ -456,8 +509,10 @@ public class RestApi {
     }
 
     public boolean isConfigLoaded() {
+        synchronized(mConfigLock) {
             return mConfig != null;
         }
+    }
 
     /**
      * Requests and parses system version information.
@@ -611,6 +666,8 @@ public class RestApi {
             return;
         }
         options.urAccepted = acceptUsageReporting ? mUrVersionMax : Options.USAGE_REPORTING_DENIED;
+        synchronized (mConfigLock) {
             mConfig.options = options;
         }
     }
+}