Merge pull request #1334 from microsoft/develop

Version 2.5.1
microsoft · Dec 6, 2019 · a172bee · a172bee
2 parents 3b96536 + 63e9e57
commit a172bee
Show file tree

Hide file tree

Showing 6 changed files with 67 additions and 7 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,13 @@
 # App Center SDK for Android Change Log
 
+## Version 2.5.1
+
+### App Center Crashes
+
+* **[Fix]** Validate error attachment size to avoid server error or out of memory issues (using the documented limit which is 7MB).
+
+___
+
 ## Version 2.5.0
 
 ### App Center Crashes

diff --git a/...h/src/main/java/com/microsoft/appcenter/sasquatch/listeners/SasquatchCrashesListener.java b/...h/src/main/java/com/microsoft/appcenter/sasquatch/listeners/SasquatchCrashesListener.java
@@ -22,7 +22,6 @@
 import com.microsoft.appcenter.sasquatch.util.AttachmentsUtil;
 import com.microsoft.appcenter.utils.HandlerUtils;
 
-@SuppressWarnings("TryFinallyCanBeTryWithResources")
 public class SasquatchCrashesListener extends AbstractCrashesListener {
 
     @VisibleForTesting

diff --git a/sdk/appcenter-crashes/src/main/java/com/microsoft/appcenter/crashes/Crashes.java b/sdk/appcenter-crashes/src/main/java/com/microsoft/appcenter/crashes/Crashes.java
@@ -55,6 +55,7 @@
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
+import java.util.Locale;
 import java.util.Map;
 import java.util.UUID;
 
@@ -118,6 +119,11 @@ public class Crashes extends AbstractAppCenterService {
      */
     private static final int MAX_ATTACHMENT_PER_CRASH = 2;
 
+    /**
+     * Maximum size for attachment data in bytes.
+     */
+    private static final int MAX_ATTACHMENT_SIZE = 7 * 1024 * 1024;
+
     /**
      * Default crashes listener.
      */
@@ -1016,11 +1022,15 @@ private void sendErrorAttachment(UUID errorId, Iterable<ErrorAttachmentLog> atta
                 if (attachment != null) {
                     attachment.setId(UUID.randomUUID());
                     attachment.setErrorId(errorId);
-                    if (attachment.isValid()) {
+                    if (!attachment.isValid()) {
+                        AppCenterLog.error(LOG_TAG, "Not all required fields are present in ErrorAttachmentLog.");
+                    } else if (attachment.getData().length > MAX_ATTACHMENT_SIZE) {
+                        AppCenterLog.error(LOG_TAG, String.format(Locale.ENGLISH,
+                                "Discarding attachment with size above %d bytes: size=%d, fileName=%s.",
+                                MAX_ATTACHMENT_SIZE, attachment.getData().length, attachment.getFileName()));
+                    } else {
                         ++totalErrorAttachments;
                         mChannel.enqueue(attachment, ERROR_GROUP, Flags.DEFAULTS);
-                    } else {
-                        AppCenterLog.error(LOG_TAG, "Not all required fields are present in ErrorAttachmentLog.");
                     }
                 } else {
                     AppCenterLog.warn(LOG_TAG, "Skipping null ErrorAttachmentLog.");

diff --git a/...es/src/main/java/com/microsoft/appcenter/crashes/ingestion/models/ErrorAttachmentLog.java b/...es/src/main/java/com/microsoft/appcenter/crashes/ingestion/models/ErrorAttachmentLog.java
@@ -204,6 +204,7 @@ public boolean isValid() {
         return getId() != null && getErrorId() != null && getContentType() != null && getData() != null;
     }
 
+    @SuppressWarnings("ConstantConditions")
     @Override
     public void read(JSONObject object) throws JSONException {
         super.read(object);
@@ -228,7 +229,7 @@ public void write(JSONStringer writer) throws JSONException {
         JSONUtils.write(writer, DATA, Base64.encodeToString(getData(), Base64.NO_WRAP));
     }
 
-    @SuppressWarnings("SimplifiableIfStatement")
+    @SuppressWarnings({"SimplifiableIfStatement", "EqualsReplaceableByObjectsCall"})
     @Override
     public boolean equals(Object o) {
         if (this == o) {

diff --git a/sdk/appcenter-crashes/src/test/java/com/microsoft/appcenter/crashes/CrashesTest.java b/sdk/appcenter-crashes/src/test/java/com/microsoft/appcenter/crashes/CrashesTest.java
@@ -73,6 +73,7 @@
 import static com.microsoft.appcenter.Constants.WRAPPER_SDK_NAME_NDK;
 import static com.microsoft.appcenter.Flags.CRITICAL;
 import static com.microsoft.appcenter.Flags.DEFAULTS;
+import static com.microsoft.appcenter.Flags.NORMAL;
 import static com.microsoft.appcenter.crashes.Crashes.PREF_KEY_MEMORY_RUNNING_LEVEL;
 import static com.microsoft.appcenter.crashes.ingestion.models.ErrorAttachmentLog.attachmentWithBinary;
 import static org.junit.Assert.assertEquals;
@@ -924,6 +925,7 @@ public void sendMoreThan2ErrorAttachments() throws JSONException {
         for (int i = 0; i < numOfAttachments; ++i) {
             ErrorAttachmentLog log = mock(ErrorAttachmentLog.class);
             when(log.isValid()).thenReturn(true);
+            when(log.getData()).thenReturn(new byte[1]);
             errorAttachmentLogs.add(log);
         }
 
@@ -957,6 +959,46 @@ public void sendMoreThan2ErrorAttachments() throws JSONException {
         AppCenterLog.warn(Crashes.LOG_TAG, expectedMessage);
     }
 
+    @Test
+    public void discardHugeErrorAttachments() throws JSONException {
+
+        /* Prepare a big (too big) attachment and a small one. */
+        ArrayList<ErrorAttachmentLog> errorAttachmentLogs = new ArrayList<>(2);
+        ErrorAttachmentLog binaryAttachment = attachmentWithBinary(new byte[7 * 1024 * 1024 + 1], "earth.png", "image/png");
+        errorAttachmentLogs.add(binaryAttachment);
+        ErrorAttachmentLog textAttachment = ErrorAttachmentLog.attachmentWithText("hello", "log.txt");
+        errorAttachmentLogs.add(textAttachment);
+
+        /* Set up callbacks. */
+        CrashesListener listener = mock(CrashesListener.class);
+        when(listener.shouldProcess(any(ErrorReport.class))).thenReturn(true);
+        when(listener.getErrorAttachments(any(ErrorReport.class))).thenReturn(errorAttachmentLogs);
+
+        /* Mock a crash log to process. */
+        ManagedErrorLog log = mock(ManagedErrorLog.class);
+        when(log.getId()).thenReturn(UUID.randomUUID());
+        LogSerializer logSerializer = mock(LogSerializer.class);
+        when(logSerializer.deserializeLog(anyString(), anyString())).thenReturn(log);
+        mockStatic(ErrorLogHelper.class);
+        when(ErrorLogHelper.getStoredErrorLogFiles()).thenReturn(new File[]{mock(File.class)});
+        when(ErrorLogHelper.getNewMinidumpFiles()).thenReturn(new File[0]);
+        when(ErrorLogHelper.getStoredThrowableFile(any(UUID.class))).thenReturn(mock(File.class));
+        when(ErrorLogHelper.getErrorReportFromErrorLog(any(ManagedErrorLog.class), anyString())).thenReturn(new ErrorReport());
+        when(FileManager.read(any(File.class))).thenReturn("");
+
+        /* Mock starting crashes so that attachments are processed. */
+        Crashes crashes = Crashes.getInstance();
+        crashes.setInstanceListener(listener);
+        crashes.setLogSerializer(logSerializer);
+        crashes.onStarting(mAppCenterHandler);
+        Channel channel = mock(Channel.class);
+        crashes.onStarted(mock(Context.class), channel, "", null, true);
+
+        /* Check we send only the text attachment as the binary is too big. */
+        verify(channel).enqueue(textAttachment, crashes.getGroupName(), NORMAL);
+        verify(channel, never()).enqueue(eq(binaryAttachment), anyString(), anyInt());
+    }
+
     @Test
     public void manualProcessing() throws Exception {
 

diff --git a/versions.gradle b/versions.gradle
@@ -6,8 +6,8 @@
 // Version constants
 
 ext {
-    versionCode = 51
-    versionName = '2.5.0'
+    versionCode = 52
+    versionName = '2.5.1'
     minSdkVersion = 16
     targetSdkVersion = 29
     compileSdkVersion = 29