From 73b0c4ea8116c4b1088dcd84cb2f4d7181baf7ab Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Mon, 30 Apr 2018 05:00:51 +0200 Subject: TunnelEditorFragment: rewrite and simplify This should remove some null pointer dereferences and overall make the thing more robust. Signed-off-by: Jason A. Donenfeld --- .../android/fragment/TunnelEditorFragment.java | 186 +++++++++------------ .../android/fragment/TunnelListFragment.java | 1 + app/src/main/java/com/wireguard/config/Config.java | 79 ++++----- app/src/main/java/com/wireguard/config/IPCidr.java | 42 +---- .../main/java/com/wireguard/config/Interface.java | 88 +++++----- app/src/main/java/com/wireguard/config/Peer.java | 82 ++++----- 6 files changed, 199 insertions(+), 279 deletions(-) (limited to 'app/src/main/java/com') diff --git a/app/src/main/java/com/wireguard/android/fragment/TunnelEditorFragment.java b/app/src/main/java/com/wireguard/android/fragment/TunnelEditorFragment.java index 0b4d74d8..e4555512 100644 --- a/app/src/main/java/com/wireguard/android/fragment/TunnelEditorFragment.java +++ b/app/src/main/java/com/wireguard/android/fragment/TunnelEditorFragment.java @@ -35,65 +35,50 @@ public class TunnelEditorFragment extends BaseFragment { private static final String TAG = "WireGuard/" + TunnelEditorFragment.class.getSimpleName(); private TunnelEditorFragmentBinding binding; - private boolean isViewStateRestored; - private Config localConfig = new Config(); - private Tunnel localTunnel; - private String originalName; - - private static T copyParcelable(final T original) { - if (original == null) - return null; - final Parcel parcel = Parcel.obtain(); - parcel.writeParcelable(original, 0); - parcel.setDataPosition(0); - final T copy = parcel.readParcelable(original.getClass().getClassLoader()); - parcel.recycle(); - return copy; + private Tunnel tunnel; + + private void onConfigLoaded(final String name, final Config config) { + binding.setConfig(new Config.Observable(config, name)); } - private void onConfigLoaded(final Config config) { - localConfig = copyParcelable(config); - if (binding != null && isViewStateRestored) - binding.setConfig(new Config.Observable(localConfig, originalName)); + @Override + public void onCreate(final Bundle savedInstanceState) { + super.onCreate(savedInstanceState); + setHasOptionsMenu(true); } - private void onConfigSaved(@SuppressWarnings("unused") final Config config, - final Throwable throwable) { - final String message; - if (throwable == null) { - message = getString(R.string.config_save_success, localTunnel.getName()); - Log.d(TAG, message); - onFinished(); - } else { - final String error = ExceptionLoggers.unwrap(throwable).getMessage(); - message = getString(R.string.config_save_error, localTunnel.getName(), error); - Log.e(TAG, message, throwable); - if (binding != null) { - final CoordinatorLayout container = binding.mainContainer; - Snackbar.make(container, message, Snackbar.LENGTH_LONG).show(); - } - } + @Override + public void onSelectedTunnelChanged(final Tunnel oldTunnel, final Tunnel newTunnel) { + tunnel = newTunnel; + if (binding == null) + return; + binding.setConfig(new Config.Observable(null, null)); + if (tunnel != null) + tunnel.getConfigAsync().thenAccept(a -> onConfigLoaded(tunnel.getName(), a)); } @Override - public void onCreate(final Bundle savedInstanceState) { - super.onCreate(savedInstanceState); - if (savedInstanceState != null) { - localConfig = savedInstanceState.getParcelable(KEY_LOCAL_CONFIG); - originalName = savedInstanceState.getString(KEY_ORIGINAL_NAME); - } - // Erase the remains of creating or editing a different tunnel. - if (getSelectedTunnel() != null && !getSelectedTunnel().getName().equals(originalName)) { - // The config must be loaded asynchronously since it's not an observable property. - localConfig = null; - originalName = getSelectedTunnel().getName(); - getSelectedTunnel().getConfigAsync().thenAccept(this::onConfigLoaded); - } else if (getSelectedTunnel() == null && originalName != null) { - localConfig = new Config(); - originalName = null; + public void onSaveInstanceState(@NonNull final Bundle outState) { + outState.putParcelable(KEY_LOCAL_CONFIG, binding.getConfig()); + outState.putString(KEY_ORIGINAL_NAME, tunnel == null ? null : tunnel.getName()); + super.onSaveInstanceState(outState); + } + + @Override + public void onViewStateRestored(final Bundle savedInstanceState) { + if (savedInstanceState == null) { + onSelectedTunnelChanged(null, getSelectedTunnel()); + } else { + tunnel = getSelectedTunnel(); + Config.Observable config = savedInstanceState.getParcelable(KEY_LOCAL_CONFIG); + String originalName = savedInstanceState.getString(KEY_ORIGINAL_NAME); + if (tunnel != null && !tunnel.getName().equals(originalName)) + onSelectedTunnelChanged(null, tunnel); + else + binding.setConfig(config); } - localTunnel = getSelectedTunnel(); - setHasOptionsMenu(true); + + super.onViewStateRestored(savedInstanceState); } @Override @@ -131,7 +116,7 @@ public class TunnelEditorFragment extends BaseFragment { // Tell the activity to finish itself or go back to the detail view. getActivity().runOnUiThread(() -> { // The selected tunnel has to actually change, but we have to remember this one. - final Tunnel savedTunnel = localTunnel; + final Tunnel savedTunnel = tunnel; if (savedTunnel == getSelectedTunnel()) setSelectedTunnel(null); setSelectedTunnel(savedTunnel); @@ -142,32 +127,31 @@ public class TunnelEditorFragment extends BaseFragment { public boolean onOptionsItemSelected(final MenuItem item) { switch (item.getItemId()) { case R.id.menu_action_save: - final Tunnel selectedTunnel = getSelectedTunnel(); - if (localConfig != null) { - try { - binding.getConfig().commitData(localConfig); - } catch (Exception e) { - final String error = ExceptionLoggers.unwrap(e).getMessage(); - final String message = getString(R.string.config_save_error, localTunnel.getName(), error); - Log.e(TAG, message, e); - final CoordinatorLayout container = binding.mainContainer; - Snackbar.make(container, error, Snackbar.LENGTH_LONG).show(); - return false; - } + Config newConfig = new Config(); + try { + binding.getConfig().commitData(newConfig); + } catch (Exception e) { + final String error = ExceptionLoggers.unwrap(e).getMessage(); + final String tunnelName = tunnel == null ? binding.getConfig().getName() : tunnel.getName(); + final String message = getString(R.string.config_save_error, tunnelName, error); + Log.e(TAG, message, e); + final CoordinatorLayout container = binding.mainContainer; + Snackbar.make(container, error, Snackbar.LENGTH_LONG).show(); + return false; } - if (selectedTunnel == null) { + if (tunnel == null) { Log.d(TAG, "Attempting to create new tunnel " + binding.getConfig().getName()); final TunnelManager manager = Application.getComponent().getTunnelManager(); - manager.create(binding.getConfig().getName(), localConfig) + manager.create(binding.getConfig().getName(), newConfig) .whenComplete(this::onTunnelCreated); - } else if (!selectedTunnel.getName().equals(binding.getConfig().getName())) { + } else if (!tunnel.getName().equals(binding.getConfig().getName())) { Log.d(TAG, "Attempting to rename tunnel to " + binding.getConfig().getName()); - selectedTunnel.setName(binding.getConfig().getName()) - .whenComplete(this::onTunnelRenamed); - } else if (localConfig != null) { - Log.d(TAG, "Attempting to save config of " + selectedTunnel.getName()); - selectedTunnel.setConfig(localConfig) - .whenComplete(this::onConfigSaved); + tunnel.setName(binding.getConfig().getName()) + .whenComplete((a, b) -> onTunnelRenamed(tunnel, newConfig, a, b)); + } else { + Log.d(TAG, "Attempting to save config of " + tunnel.getName()); + tunnel.setConfig(newConfig) + .whenComplete((a, b) -> onConfigSaved(tunnel, a, b)); } return true; default: @@ -175,36 +159,30 @@ public class TunnelEditorFragment extends BaseFragment { } } - @Override - public void onSaveInstanceState(@NonNull final Bundle outState) { - outState.putParcelable(KEY_LOCAL_CONFIG, localConfig); - outState.putString(KEY_ORIGINAL_NAME, originalName); - super.onSaveInstanceState(outState); - } - - @Override - public void onSelectedTunnelChanged(final Tunnel oldTunnel, final Tunnel newTunnel) { - // Erase the remains of creating or editing a different tunnel. - if (newTunnel != null) { - // The config must be loaded asynchronously since it's not an observable property. - localConfig = null; - originalName = newTunnel.getName(); - newTunnel.getConfigAsync().thenAccept(this::onConfigLoaded); + private void onConfigSaved(final Tunnel savedTunnel, final Config config, + final Throwable throwable) { + final String message; + if (throwable == null) { + message = getString(R.string.config_save_success, savedTunnel.getName()); + Log.d(TAG, message); + onFinished(); } else { - localConfig = new Config(); - if (binding != null && isViewStateRestored) - binding.setConfig(new Config.Observable(localConfig, "")); - originalName = null; + final String error = ExceptionLoggers.unwrap(throwable).getMessage(); + message = getString(R.string.config_save_error, savedTunnel.getName(), error); + Log.e(TAG, message, throwable); + if (binding != null) { + final CoordinatorLayout container = binding.mainContainer; + Snackbar.make(container, message, Snackbar.LENGTH_LONG).show(); + } } - localTunnel = newTunnel; } - private void onTunnelCreated(final Tunnel tunnel, final Throwable throwable) { + private void onTunnelCreated(final Tunnel newTunnel, final Throwable throwable) { final String message; if (throwable == null) { + tunnel = newTunnel; message = getString(R.string.tunnel_create_success, tunnel.getName()); Log.d(TAG, message); - localTunnel = tunnel; onFinished(); } else { final String error = ExceptionLoggers.unwrap(throwable).getMessage(); @@ -217,14 +195,15 @@ public class TunnelEditorFragment extends BaseFragment { } } - private void onTunnelRenamed(final String name, final Throwable throwable) { + private void onTunnelRenamed(final Tunnel renamedTunnel, final Config newConfig, + final String name, final Throwable throwable) { final String message; if (throwable == null) { - message = getString(R.string.tunnel_rename_success, localTunnel.getName(), name); + message = getString(R.string.tunnel_rename_success, renamedTunnel.getName()); Log.d(TAG, message); // Now save the rest of configuration changes. - Log.d(TAG, "Attempting to save config of renamed tunnel " + localTunnel.getName()); - localTunnel.setConfig(localConfig).whenComplete(this::onConfigSaved); + Log.d(TAG, "Attempting to save config of renamed tunnel " + tunnel.getName()); + renamedTunnel.setConfig(newConfig).whenComplete((a, b) -> onConfigSaved(renamedTunnel, a, b)); } else { final String error = ExceptionLoggers.unwrap(throwable).getMessage(); message = getString(R.string.tunnel_rename_error, error); @@ -235,13 +214,4 @@ public class TunnelEditorFragment extends BaseFragment { } } } - - @Override - public void onViewStateRestored(final Bundle savedInstanceState) { - super.onViewStateRestored(savedInstanceState); - if (localConfig == null) - localConfig = new Config(); - binding.setConfig(new Config.Observable(localConfig, originalName)); - isViewStateRestored = true; - } } diff --git a/app/src/main/java/com/wireguard/android/fragment/TunnelListFragment.java b/app/src/main/java/com/wireguard/android/fragment/TunnelListFragment.java index 69140c63..05eaccf1 100644 --- a/app/src/main/java/com/wireguard/android/fragment/TunnelListFragment.java +++ b/app/src/main/java/com/wireguard/android/fragment/TunnelListFragment.java @@ -25,6 +25,7 @@ import android.widget.AbsListView.MultiChoiceModeListener; import android.widget.AdapterView; import android.widget.AdapterView.OnItemClickListener; import android.widget.AdapterView.OnItemLongClickListener; +import android.widget.TextView; import com.commonsware.cwac.crossport.design.widget.CoordinatorLayout; import com.commonsware.cwac.crossport.design.widget.Snackbar; diff --git a/app/src/main/java/com/wireguard/config/Config.java b/app/src/main/java/com/wireguard/config/Config.java index b38d2a13..6330ff5e 100644 --- a/app/src/main/java/com/wireguard/config/Config.java +++ b/app/src/main/java/com/wireguard/config/Config.java @@ -21,35 +21,24 @@ import java.util.List; * Represents a wg-quick configuration file, its name, and its connection state. */ -public class Config implements Parcelable { - public static final Creator CREATOR = new Creator() { - @Override - public Config createFromParcel(final Parcel in) { - return new Config(in); - } - - @Override - public Config[] newArray(final int size) { - return new Config[size]; - } - }; - - public static class Observable extends BaseObservable { +public class Config { + public static class Observable extends BaseObservable implements Parcelable { private String name; private Interface.Observable observableInterface; private ObservableList observablePeers; - public Observable(Config parent, String name) { this.name = name; loadData(parent); } public void loadData(Config parent) { - this.observableInterface = new Interface.Observable(parent.interfaceSection); + this.observableInterface = new Interface.Observable(parent == null ? null : parent.interfaceSection); this.observablePeers = new ObservableArrayList<>(); - for (Peer peer : parent.getPeers()) - this.observablePeers.add(new Peer.Observable(peer)); + if (parent != null) { + for (Peer peer : parent.getPeers()) + this.observablePeers.add(new Peer.Observable(peer)); + } } public void commitData(Config parent) { @@ -66,7 +55,7 @@ public class Config implements Parcelable { @Bindable public String getName() { - return name; + return name == null ? "" : name; } public void setName(String name) { @@ -83,20 +72,43 @@ public class Config implements Parcelable { public ObservableList getPeers() { return observablePeers; } - } - private final Interface interfaceSection; - private List peers = new ArrayList<>(); - public Config() { - interfaceSection = new Interface(); - } + public static final Creator CREATOR = new Creator() { + @Override + public Observable createFromParcel(final Parcel in) { + return new Observable(in); + } - private Config(final Parcel in) { - interfaceSection = in.readParcelable(Interface.class.getClassLoader()); - in.readTypedList(peers, Peer.CREATOR); + @Override + public Observable[] newArray(final int size) { + return new Observable[size]; + } + }; + + @Override + public int describeContents() { + return 0; + } + + @Override + public void writeToParcel(final Parcel dest, final int flags) { + dest.writeString(name); + dest.writeParcelable(observableInterface, flags); + dest.writeTypedList(observablePeers); + } + + private Observable(final Parcel in) { + name = in.readString(); + observableInterface = in.readParcelable(Interface.Observable.class.getClassLoader()); + observablePeers = new ObservableArrayList<>(); + in.readTypedList(observablePeers, Peer.Observable.CREATOR); + } } + private final Interface interfaceSection = new Interface(); + private List peers = new ArrayList<>(); + public static Config from(final InputStream stream) throws IOException { return from(new BufferedReader(new InputStreamReader(stream, StandardCharsets.UTF_8))); } @@ -130,11 +142,6 @@ public class Config implements Parcelable { return config; } - @Override - public int describeContents() { - return 0; - } - public Interface getInterface() { return interfaceSection; } @@ -150,10 +157,4 @@ public class Config implements Parcelable { sb.append('\n').append(peer); return sb.toString(); } - - @Override - public void writeToParcel(final Parcel dest, final int flags) { - dest.writeParcelable(interfaceSection, flags); - dest.writeTypedList(peers); - } } diff --git a/app/src/main/java/com/wireguard/config/IPCidr.java b/app/src/main/java/com/wireguard/config/IPCidr.java index 569fd410..41d43cd6 100644 --- a/app/src/main/java/com/wireguard/config/IPCidr.java +++ b/app/src/main/java/com/wireguard/config/IPCidr.java @@ -1,35 +1,15 @@ package com.wireguard.config; -import android.os.Parcel; -import android.os.Parcelable; - import java.net.Inet4Address; import java.net.Inet6Address; import java.net.InetAddress; import java.util.Locale; -public class IPCidr implements Parcelable { +public class IPCidr { private InetAddress address; private int cidr; - - public static final Parcelable.Creator CREATOR = new Parcelable.Creator() { - @Override - public IPCidr createFromParcel(final Parcel in) { - return new IPCidr(in); - } - - @Override - public IPCidr[] newArray(final int size) { - return new IPCidr[size]; - } - }; - - IPCidr(String in) { - parse(in); - } - - private void parse(String in) { + public IPCidr(String in) { cidr = -1; int slash = in.lastIndexOf('/'); if (slash != -1 && slash < in.length() - 1) { @@ -58,22 +38,4 @@ public class IPCidr implements Parcelable { public String toString() { return String.format(Locale.getDefault(), "%s/%d", address.getHostAddress(), cidr); } - - @Override - public void writeToParcel(final Parcel dest, final int flags) { - dest.writeString(this.toString()); - } - - @Override - public int describeContents() { - return 0; - } - - private IPCidr(final Parcel in) { - try { - parse(in.readString()); - } catch (Exception ignored) { - } - } - } diff --git a/app/src/main/java/com/wireguard/config/Interface.java b/app/src/main/java/com/wireguard/config/Interface.java index 5b125f51..758b528d 100644 --- a/app/src/main/java/com/wireguard/config/Interface.java +++ b/app/src/main/java/com/wireguard/config/Interface.java @@ -6,7 +6,6 @@ import android.os.Parcel; import android.os.Parcelable; import com.wireguard.android.BR; -import com.wireguard.crypto.KeyEncoding; import com.wireguard.crypto.Keypair; import java.net.InetAddress; @@ -17,20 +16,8 @@ import java.util.List; * Represents the configuration for a WireGuard interface (an [Interface] block). */ -public class Interface implements Parcelable { - public static final Creator CREATOR = new Creator() { - @Override - public Interface createFromParcel(final Parcel in) { - return new Interface(in); - } - - @Override - public Interface[] newArray(final int size) { - return new Interface[size]; - } - }; - - public static class Observable extends BaseObservable { +public class Interface { + public static class Observable extends BaseObservable implements Parcelable { private String addresses; private String dnses; private String publicKey; @@ -39,7 +26,8 @@ public class Interface implements Parcelable { private String mtu; public Observable(Interface parent) { - loadData(parent); + if (parent != null) + loadData(parent); } public void loadData(Interface parent) { @@ -131,6 +119,43 @@ public class Interface implements Parcelable { this.mtu = mtu; notifyPropertyChanged(BR.mtu); } + + + public static final Creator CREATOR = new Creator() { + @Override + public Observable createFromParcel(final Parcel in) { + return new Observable(in); + } + + @Override + public Observable[] newArray(final int size) { + return new Observable[size]; + } + }; + + @Override + public int describeContents() { + return 0; + } + + @Override + public void writeToParcel(final Parcel dest, final int flags) { + dest.writeString(addresses); + dest.writeString(dnses); + dest.writeString(publicKey); + dest.writeString(privateKey); + dest.writeString(listenPort); + dest.writeString(mtu); + } + + private Observable(final Parcel in) { + addresses = in.readString(); + dnses = in.readString(); + publicKey = in.readString(); + privateKey = in.readString(); + listenPort = in.readString(); + mtu = in.readString(); + } } private List addressList; @@ -144,26 +169,6 @@ public class Interface implements Parcelable { dnsList = new LinkedList<>(); } - private Interface(final Parcel in) { - addressList = in.createTypedArrayList(IPCidr.CREATOR); - int dnsItems = in.readInt(); - dnsList = new LinkedList<>(); - for (int i = 0; i < dnsItems; ++i) { - try { - dnsList.add(InetAddress.getByAddress(in.createByteArray())); - } catch (Exception ignored) { - } - } - listenPort = in.readInt(); - mtu = in.readInt(); - setPrivateKey(in.readString()); - } - - @Override - public int describeContents() { - return 0; - } - private String getAddressString() { if (addressList.isEmpty()) return null; @@ -313,15 +318,4 @@ public class Interface implements Parcelable { sb.append(Attribute.PRIVATE_KEY.composeWith(keypair.getPrivateKey())); return sb.toString(); } - - @Override - public void writeToParcel(final Parcel dest, final int flags) { - dest.writeTypedList(addressList); - dest.writeInt(dnsList.size()); - for (final InetAddress addr : dnsList) - dest.writeByteArray(addr.getAddress()); - dest.writeInt(listenPort); - dest.writeInt(mtu); - dest.writeString(keypair == null ? "" : keypair.getPrivateKey()); - } } diff --git a/app/src/main/java/com/wireguard/config/Peer.java b/app/src/main/java/com/wireguard/config/Peer.java index 28495f46..327365b3 100644 --- a/app/src/main/java/com/wireguard/config/Peer.java +++ b/app/src/main/java/com/wireguard/config/Peer.java @@ -21,27 +21,14 @@ import java.util.Locale; * Represents the configuration for a WireGuard peer (a [Peer] block). */ -public class Peer implements Parcelable { - public static final Creator CREATOR = new Creator() { - @Override - public Peer createFromParcel(final Parcel in) { - return new Peer(in); - } - - @Override - public Peer[] newArray(final int size) { - return new Peer[size]; - } - }; - - public static class Observable extends BaseObservable { +public class Peer { + public static class Observable extends BaseObservable implements Parcelable { private String allowedIPs; private String endpoint; private String persistentKeepalive; private String preSharedKey; private String publicKey; - public Observable(Peer parent) { loadData(parent); } @@ -118,6 +105,41 @@ public class Peer implements Parcelable { this.publicKey = publicKey; notifyPropertyChanged(BR.publicKey); } + + + public static final Creator CREATOR = new Creator() { + @Override + public Observable createFromParcel(final Parcel in) { + return new Observable(in); + } + + @Override + public Observable[] newArray(final int size) { + return new Observable[size]; + } + }; + + @Override + public int describeContents() { + return 0; + } + + @Override + public void writeToParcel(final Parcel dest, final int flags) { + dest.writeString(allowedIPs); + dest.writeString(endpoint); + dest.writeString(persistentKeepalive); + dest.writeString(preSharedKey); + dest.writeString(publicKey); + } + + private Observable(final Parcel in) { + allowedIPs = in.readString(); + endpoint = in.readString(); + persistentKeepalive = in.readString(); + preSharedKey = in.readString(); + publicKey = in.readString(); + } } private List allowedIPsList; @@ -130,26 +152,6 @@ public class Peer implements Parcelable { allowedIPsList = new LinkedList<>(); } - private Peer(final Parcel in) { - allowedIPsList = in.createTypedArrayList(IPCidr.CREATOR); - String host = in.readString(); - int port = in.readInt(); - if (host != null && !host.isEmpty() && port > 0) - endpoint = InetSocketAddress.createUnresolved(host, port); - persistentKeepalive = in.readInt(); - preSharedKey = in.readString(); - if (preSharedKey != null && preSharedKey.isEmpty()) - preSharedKey = null; - publicKey = in.readString(); - if (publicKey != null && publicKey.isEmpty()) - publicKey = null; - } - - @Override - public int describeContents() { - return 0; - } - private String getAllowedIPsString() { if (allowedIPsList.isEmpty()) @@ -299,14 +301,4 @@ public class Peer implements Parcelable { sb.append(Attribute.PUBLIC_KEY.composeWith(publicKey)); return sb.toString(); } - - @Override - public void writeToParcel(final Parcel dest, final int flags) { - dest.writeTypedList(allowedIPsList); - dest.writeString(endpoint == null ? null : endpoint.getHostString()); - dest.writeInt(endpoint == null ? 0 : endpoint.getPort()); - dest.writeInt(persistentKeepalive); - dest.writeString(preSharedKey); - dest.writeString(publicKey); - } } -- cgit v1.2.3-59-g8ed1b