From b50fcf1fa140038bae094ab025f9790933ea6b4b Mon Sep 17 00:00:00 2001 From: Catfriend1 Date: Sun, 29 Jul 2018 18:25:56 +0200 Subject: [PATCH] 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 --- .../fragments/DeviceListFragment.java | 17 +- .../fragments/FolderListFragment.java | 17 +- .../syncthingandroid/service/RestApi.java | 159 ++++++++++++------ 3 files changed, 132 insertions(+), 61 deletions(-) diff --git a/app/src/main/java/com/nutomic/syncthingandroid/fragments/DeviceListFragment.java b/app/src/main/java/com/nutomic/syncthingandroid/fragments/DeviceListFragment.java index 54cc5d3e..5caff4db 100644 --- a/app/src/main/java/com/nutomic/syncthingandroid/fragments/DeviceListFragment.java +++ b/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() || - getView() == null || activity.isFinishing()) + if (activity == null || getView() == null || activity.isFinishing()) { return; - + } + RestApi restApi = activity.getApi(); + if (restApi == null || !restApi.isConfigLoaded()) { + return; + } + List 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 devices = activity.getApi().getDevices(false); Collections.sort(devices, DEVICES_COMPARATOR); mAdapter.addAll(devices); - mAdapter.updateConnections(activity.getApi()); + mAdapter.updateConnections(restApi); mAdapter.notifyDataSetChanged(); setListShown(true); } diff --git a/app/src/main/java/com/nutomic/syncthingandroid/fragments/FolderListFragment.java b/app/src/main/java/com/nutomic/syncthingandroid/fragments/FolderListFragment.java index 72212a50..ca84ed1f 100644 --- a/app/src/main/java/com/nutomic/syncthingandroid/fragments/FolderListFragment.java +++ b/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() || - getView() == null || activity.isFinishing()) + if (activity == null || getView() == null || activity.isFinishing()) { return; - + } + RestApi restApi = activity.getApi(); + if (restApi == null || !restApi.isConfigLoaded()) { + return; + } + List 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 folders = activity.getApi().getFolders(); mAdapter.addAll(folders); - mAdapter.updateFolderStatus(activity.getApi()); + mAdapter.updateFolderStatus(restApi); mAdapter.notifyDataSetChanged(); setListShown(true); } diff --git a/app/src/main/java/com/nutomic/syncthingandroid/service/RestApi.java b/app/src/main/java/com/nutomic/syncthingandroid/service/RestApi.java index f1f59648..65998bdf 100644 --- a/app/src/main/java/com/nutomic/syncthingandroid/service/RestApi.java +++ b/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) { - mConfig = new Gson().fromJson(result, Config.class); - if (mConfig == null) { + Boolean configParseSuccess; + synchronized(mConfigLock) { + mConfig = new Gson().fromJson(result, Config.class); + configParseSuccess = mConfig != null; + } + if (!configParseSuccess) { throw new RuntimeException("config is null: " + result); } Log.v(TAG, "onReloadConfigComplete: Successfully parsed configuration."); @@ -249,10 +258,12 @@ public class RestApi { * in {@link EventProcessor#onEvent}. */ public void ignoreDevice(String deviceId) { - if (!mConfig.ignoredDevices.contains(deviceId)) { - mConfig.ignoredDevices.add(deviceId); - sendConfig(); - Log.d(TAG, "Ignored device [" + deviceId + "]"); + synchronized (mConfigLock) { + if (!mConfig.ignoredDevices.contains(deviceId)) { + mConfig.ignoredDevices.add(deviceId); + sendConfig(); + Log.d(TAG, "Ignored device [" + deviceId + "]"); + } } } @@ -262,10 +273,12 @@ public class RestApi { * in {@link EventProcessor#onEvent}. */ public void ignoreFolder(String folderId) { - if (!mConfig.ignoredFolders.contains(folderId)) { - mConfig.ignoredFolders.add(folderId); - sendConfig(); - Log.d(TAG, "Ignored folder [" + folderId + "]"); + synchronized (mConfigLock) { + if (!mConfig.ignoredFolders.contains(folderId)) { + mConfig.ignoredFolders.add(folderId); + sendConfig(); + Log.d(TAG, "Ignored folder [" + folderId + "]"); + } } } @@ -274,8 +287,10 @@ public class RestApi { */ public void undoIgnoredDevicesAndFolders() { Log.d(TAG, "Undo ignoring devices and folders ..."); - mConfig.ignoredDevices.clear(); - mConfig.ignoredFolders.clear(); + synchronized (mConfigLock) { + mConfig.ignoredDevices.clear(); + mConfig.ignoredFolders.clear(); + } } /** @@ -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 getFolders() { - List folders = deepCopy(mConfig.folders, new TypeToken>(){}.getType()); + List folders; + synchronized (mConfigLock) { + folders = deepCopy(mConfig.folders, new TypeToken>(){}.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) { - // Add the new folder to the model. - mConfig.folders.add(folder); - // Send model changes to syncthing, does not require a restart. - sendConfig(); + 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) { - removeFolderInternal(newFolder.id); - mConfig.folders.add(newFolder); - sendConfig(); + synchronized (mConfigLock) { + removeFolderInternal(newFolder.id); + mConfig.folders.add(newFolder); + sendConfig(); + } } public void removeFolder(String id) { - removeFolderInternal(id); - // mCompletion will be updated after the ConfigSaved event. - sendConfig(); - // Remove saved data from share activity for this folder. + 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) { - Iterator it = mConfig.folders.iterator(); - while (it.hasNext()) { - Folder f = it.next(); - if (f.id.equals(id)) { - it.remove(); + synchronized (mConfigLock) { + Iterator 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 getDevices(boolean includeLocal) { - List devices = deepCopy(mConfig.devices, new TypeToken>(){}.getType()); + List devices; + synchronized (mConfigLock) { + devices = deepCopy(mConfig.devices, new TypeToken>(){}.getType()); + } Iterator it = devices.iterator(); while (it.hasNext()) { @@ -397,44 +435,59 @@ public class RestApi { public void addDevice(Device device, OnResultListener1 errorListener) { normalizeDeviceId(device.deviceID, normalizedId -> { - mConfig.devices.add(device); - sendConfig(); + synchronized (mConfigLock) { + mConfig.devices.add(device); + sendConfig(); + } }, errorListener); } public void editDevice(Device newDevice) { - removeDeviceInternal(newDevice.deviceID); - mConfig.devices.add(newDevice); - sendConfig(); + synchronized (mConfigLock) { + removeDeviceInternal(newDevice.deviceID); + mConfig.devices.add(newDevice); + sendConfig(); + } } public void removeDevice(String deviceId) { - removeDeviceInternal(deviceId); - // mCompletion will be updated after the ConfigSaved event. - sendConfig(); + synchronized (mConfigLock) { + removeDeviceInternal(deviceId); + // mCompletion will be updated after the ConfigSaved event. + sendConfig(); + } } private void removeDeviceInternal(String deviceId) { - Iterator it = mConfig.devices.iterator(); - while (it.hasNext()) { - Device d = it.next(); - if (d.deviceID.equals(deviceId)) { - it.remove(); + synchronized (mConfigLock) { + Iterator it = mConfig.devices.iterator(); + while (it.hasNext()) { + Device d = it.next(); + if (d.deviceID.equals(deviceId)) { + it.remove(); + break; + } } } } public Options getOptions() { - return deepCopy(mConfig.options, Options.class); + synchronized (mConfigLock) { + return deepCopy(mConfig.options, Options.class); + } } public Config.Gui getGui() { - return deepCopy(mConfig.gui, Config.Gui.class); + synchronized (mConfigLock) { + return deepCopy(mConfig.gui, Config.Gui.class); + } } public void editSettings(Config.Gui newGui, Options newOptions) { - mConfig.gui = newGui; - mConfig.options = newOptions; + synchronized (mConfigLock) { + mConfig.gui = newGui; + mConfig.options = newOptions; + } } /** @@ -456,7 +509,9 @@ public class RestApi { } public boolean isConfigLoaded() { - return mConfig != null; + synchronized(mConfigLock) { + return mConfig != null; + } } /** @@ -611,6 +666,8 @@ public class RestApi { return; } options.urAccepted = acceptUsageReporting ? mUrVersionMax : Options.USAGE_REPORTING_DENIED; - mConfig.options = options; + synchronized (mConfigLock) { + mConfig.options = options; + } } }