Skip to content

Do not attempt to parse empty json files in MetaDataStore #3735

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 18, 2022
Merged

Do not attempt to parse empty json files in MetaDataStore #3735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16495ae
Do not attempt to parse empty json files in MetaDataStore
mrober May 18, 2022
5954eeb
Safely delete corrupt files in MetaDataStore
mrober May 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 42 additions & 2 deletions ...androidTest/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,14 @@
import com.google.firebase.crashlytics.internal.CrashlyticsTestCase;
import com.google.firebase.crashlytics.internal.common.CrashlyticsBackgroundWorker;
import com.google.firebase.crashlytics.internal.persistence.FileStore;
import java.io.File;
import java.io.IOException;
import java.io.PrintWriter;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

@SuppressWarnings("ResultOfMethodCallIgnored") // Convenient use of files.
public class MetaDataStoreTest extends CrashlyticsTestCase {

private static final String SESSION_ID_1 = "session1";
Expand All @@ -40,7 +44,7 @@ public class MetaDataStoreTest extends CrashlyticsTestCase {
private static final String ESCAPED = "\ttest\nvalue";

private FileStore fileStore;
private CrashlyticsBackgroundWorker worker = new CrashlyticsBackgroundWorker(Runnable::run);
private final CrashlyticsBackgroundWorker worker = new CrashlyticsBackgroundWorker(Runnable::run);

private MetaDataStore storeUnderTest;

Expand Down Expand Up @@ -83,7 +87,7 @@ public void testWriteUserData_singleField() {
public void testWriteUserData_null() {
storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1, null).getUserId());
UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker);
assertEquals(null, userData.getUserId());
assertNull(userData.getUserId());
}

public void testWriteUserData_emptyString() {
Expand Down Expand Up @@ -112,6 +116,24 @@ public void testWriteUserData_readDifferentSession() {
assertNull(userData.getUserId());
}

public void testReadUserData_corruptData() throws IOException {
File file = storeUnderTest.getUserDataFileForSession(SESSION_ID_1);
try (PrintWriter printWriter = new PrintWriter(file)) {
printWriter.println("Matt says hi!");
}
UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker);
assertNull(userData.getUserId());
assertFalse(file.exists());
}

public void testReadUserData_emptyData() throws IOException {
File file = storeUnderTest.getUserDataFileForSession(SESSION_ID_1);
file.createNewFile();
UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker);
assertNull(userData.getUserId());
assertFalse(file.exists());
}

public void testReadUserData_noStoredData() {
UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker);
assertNull(userData.getUserId());
Expand Down Expand Up @@ -237,6 +259,24 @@ public void testWriteKeys_readSeparateFromUser() {
assertEqualMaps(internalKeys, readInternalKeys);
}

public void testReadKeys_corruptData() throws IOException {
File file = storeUnderTest.getKeysFileForSession(SESSION_ID_1);
try (PrintWriter printWriter = new PrintWriter(file)) {
printWriter.println("This is not json.");
}
final Map<String, String> readKeys = storeUnderTest.readKeyData(SESSION_ID_1);
assertEquals(0, readKeys.size());
assertFalse(file.exists());
}

public void testReadKeys_emptyStoredData() throws IOException {
File file = storeUnderTest.getKeysFileForSession(SESSION_ID_1);
file.createNewFile();
final Map<String, String> readKeys = storeUnderTest.readKeyData(SESSION_ID_1);
assertEquals(0, readKeys.size());
assertFalse(file.exists());
}

public void testReadKeys_noStoredData() {
final Map<String, String> readKeys = storeUnderTest.readKeyData(SESSION_ID_1);
assertEquals(0, readKeys.size());
Expand Down
23 changes: 17 additions & 6 deletions ...lytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public void writeUserData(String sessionId, String userId) {
writer.write(userIdJson);
writer.flush();
} catch (Exception e) {
Logger.getLogger().e("Error serializing user metadata.", e);
Logger.getLogger().w("Error serializing user metadata.", e);
} finally {
CommonUtils.closeOrLog(writer, "Failed to close user metadata file.");
}
Expand All @@ -68,8 +68,9 @@ public void writeUserData(String sessionId, String userId) {
@Nullable
public String readUserId(String sessionId) {
final File f = getUserDataFileForSession(sessionId);
if (!f.exists()) {
if (!f.exists() || f.length() == 0) {
Logger.getLogger().d("No userId set for session " + sessionId);
safeDeleteCorruptFile(f);
return null;
}

Expand All @@ -80,7 +81,8 @@ public String readUserId(String sessionId) {
Logger.getLogger().d("Loaded userId " + userId + " for session " + sessionId);
return userId;
} catch (Exception e) {
Logger.getLogger().e("Error deserializing user metadata.", e);
Logger.getLogger().w("Error deserializing user metadata.", e);
safeDeleteCorruptFile(f);
} finally {
CommonUtils.closeOrLog(is, "Failed to close user metadata file.");
}
Expand All @@ -101,7 +103,8 @@ public void writeKeyData(String sessionId, Map<String, String> keyData, boolean
writer.write(keyDataString);
writer.flush();
} catch (Exception e) {
Logger.getLogger().e("Error serializing key/value metadata.", e);
Logger.getLogger().w("Error serializing key/value metadata.", e);
safeDeleteCorruptFile(f);
} finally {
CommonUtils.closeOrLog(writer, "Failed to close key/value metadata file.");
}
Expand All @@ -114,7 +117,8 @@ public Map<String, String> readKeyData(String sessionId) {
Map<String, String> readKeyData(String sessionId, boolean isInternal) {
final File f =
isInternal ? getInternalKeysFileForSession(sessionId) : getKeysFileForSession(sessionId);
if (!f.exists()) {
if (!f.exists() || f.length() == 0) {
safeDeleteCorruptFile(f);
return Collections.emptyMap();
}

Expand All @@ -123,7 +127,8 @@ Map<String, String> readKeyData(String sessionId, boolean isInternal) {
is = new FileInputStream(f);
return jsonToKeysData(CommonUtils.streamToString(is));
} catch (Exception e) {
Logger.getLogger().e("Error deserializing user metadata.", e);
Logger.getLogger().w("Error deserializing user metadata.", e);
safeDeleteCorruptFile(f);
} finally {
CommonUtils.closeOrLog(is, "Failed to close user metadata file.");
}
Expand Down Expand Up @@ -177,4 +182,10 @@ private static String keysDataToJson(final Map<String, String> keyData) {
private static String valueOrNull(JSONObject json, String key) {
return !json.isNull(key) ? json.optString(key, null) : null;
}

private static void safeDeleteCorruptFile(File file) {
if (file.exists() && file.delete()) {
Logger.getLogger().i("Deleted corrupt file: " + file.getAbsolutePath());
}
}
}