Fix crashes where `setHeartbeatDate:forTag:` reads immutable dictionary. #37

ncooke3 · 2021-05-07T18:59:20Z

Fix for planned GoogleUtils 7.4.1 patch release.

NSDictionary-related type changes introduced in #19 caused crashes for app versions that used an older GoogleUtilities version on top of an app version that's using GoogleUtilities version 7.4.0.

The crash occurred in setHeartbeatDate:forTag: (for apps with GULs pre-7.4) when the API internally expected a mutable dictionary but received an immutable dictionary (introduced in 7.4.0) from persistent storage.

See a thorough explanation here.

Key fix: Always write a mutable heartbeat dictionary to disk.

Since a mutable heartbeat dictionary is now stored, updated helper dictionary getter method to always
return mutable heartbeat dictionary.

Fixes #36
Fixes #8047

Some interesting things to note when it comes to archiving/unarchiving `NSDictionary`s

tldr; keep track of what type you are archiving and unarchiving because the underlying types can make things tricky

NSData *data = // archive a `NSDictionary` instance

// Then, unarchive into an `NSMutableDictionary`
NSMutableDictionary *mutableDict = // unarchive `data`
// `mutableDict`'s underlying type is a `__NSSingleEntryDictionaryI` where 
// the trailing `I` means it's *immutable*
mutableDict[tag] = [NSDate date]; // 🔥 Crash because `mutableDict` is actually *immutable*

// ------------------------------------- //

NSData *data = // archive a `NSMutableDictionary` instance

// Then, unarchive into an `NSDictionary`
NSDictionary *immutableDict = // unarchive `data`
// `immutableDict`'s underlying type is a `__NSDictionaryM` where 
// the trailing `M` means it's *mutable*
immutableDict[tag] = [NSDate date]; // ✔ This is ok because `immutableDict` is actually *mutable*.

google-oss-bot · 2021-05-07T19:09:00Z

Coverage Report

Affected SDKs

GoogleUtilities-ios-unit-GoogleUtilities.framework

SDK overall coverage changed from 0.28% (66f5945) to 0.28% (b4733d2) by -0.00%.

Filename Base (66f5945) Head (b4733d2) Diff

Test Logs

Base (66f5945a): https://github.com/google/GoogleUtilities/actions/runs/786855872
Head (b4733d2d): https://github.com/google/GoogleUtilities/actions/runs/829913193

maksymmalyhin

LGTM in general, but I have a suggestion, let's consider it first.

maksymmalyhin · 2021-05-07T20:59:13Z

GoogleUtilities/Environment/GULHeartbeatDateStorage.m

 }

- (BOOL)writeDictionary:(NSDictionary *)dictionary
+- (BOOL)writeDictionary:(NSMutableDictionary *)dictionary
          forWritingURL:(NSURL *)writingFileURL
                  error:(NSError **)outError {
  NSData *data = [GULSecureCoding archivedDataWithRootObject:dictionary error:outError];


What if we change only writeDictionary:... method to archive the mutable dictionary (a comment that we do that for backward compatibility and reminder to eventually delete it should be added as well). Then the rest of the GULHeartbeatDateStorage.m code can remain unchanged and also we will have a good migration story because the current version of the code (7.4.0) can work with both mutable and immutable version of the file.

It should be a better option unless I'm missing something. WDYT?

So with your proposal, the only change to the library would be line 151 changed to:

NSData *data = [GULSecureCoding archivedDataWithRootObject:dictionary.mutableCopy error:outError];

SGTM

maksymmalyhin · 2021-05-07T21:02:22Z

GoogleUtilities/Tests/Unit/Environment/GULHeartbeatDateStorageTest.m

+  //    This implies the storage directory & file were created, written to, and read from.
+  XCTAssertEqualObjects(retrievedDate, date);
+}
+


I would probably add a test to check if the archive contains the mutable dictionary after the heartbeat has been updated.

maksymmalyhin

To make sure we don't end up with a similar crash in the future we should add type validations to heartbeatDictionaryWithFileURL: and heartbeatDateForTag: methods and make sure we don't crash on unexpected file content.

paulb777

Change the version in the podspec to 7.4.1

ncooke3 · 2021-05-10T14:26:18Z

GoogleUtilities/Tests/Unit/Environment/GULHeartbeatDateStorageTest.m

@@ -169,6 +169,65 @@ - (void)testHeartbeatDateForTagWhenHeartbeatFileReturnsInvalidData {
  XCTAssertNil(retrievedDate);
 }

+- (void)testHeartbeatDateForTagWhenHeartbeatFileContainsUnexpectedContent {


To make sure we don't end up with a similar crash in the future we should add type validations to heartbeatDictionaryWithFileURL: and heartbeatDateForTag: methods and make sure we don't crash on unexpected file content.

This test and the following two tests show how the hb APIs recover when the file contains unexpected content or invalid data. Are these sufficient for the cases you are describing above? @maksymmalyhin

maksymmalyhin · 2021-05-10T14:47:07Z

GoogleUtilities/Tests/Unit/Environment/GULHeartbeatDateStorageTest.m

+  NSString *tag = @"tag";
+
+  // 4. Retrieve heartbeat info that is not stored and assert the retrieved info is nil.
+  NSDate *retrievedDate = [self.storage heartbeatDateForTag:tag];


Hm... I would expect this to crash on heartbeatDictionary[tag];. But with this particular test data, I think, it doesn't because of the secure coding validation.

Let't add another invalid data set where we have e.g. a dictionary of dictionaries with dates. Handling this case will most likely require code changes.

ncooke3 · 2021-05-10T17:13:27Z

GoogleUtilities/Tests/Unit/Environment/GULHeartbeatDateStorageTest.m

+#pragma mark - GoogleUtilities Issue #36
+
+- (void)testFix_pre7dot4_CompabitibilityIssue {


Added this test to validate current fix. This test passes. If the implementation is modified with a one line edit to write an immutable dictionary to disk (effectively, reverting this PR), then this test fails with error from #36

maksymmalyhin · 2021-05-10T17:43:09Z

GoogleUtilities/Tests/Unit/Environment/GULHeartbeatDateStorageTest.m

+  // Developer downgrades from 7.4.0
+
+  // Set heartbeat from pre-7.4.0 API. Note: The relevant logic from pre-7.4.0 APIs is used below.
+  NSURL *heartbeatStorageDirectoryURL = [self pathURLForDirectory:kGULHeartbeatStorageDirectory];


I was thinking of having a full copies of GULHeartbeatDateStorage.[hm] files like GULHeartbeatDateStorageV7.3.1 and GULHeartbeatDateStorageV7.4.0, so instances of them can be created in the same test to do operations and check mutual compatibility. It seems to be the easiest and the most readable way to implement the migration tests.

paulb777 · 2021-05-10T22:46:57Z

GoogleUtilities/Tests/Unit/Environment/GULHeartbeatDateStorageTest.m

@@ -18,6 +18,10 @@
 #import "GoogleUtilities/Environment/Public/GoogleUtilities/GULHeartbeatDateStorage.h"
 #import "GoogleUtilities/Environment/Public/GoogleUtilities/GULSecureCoding.h"

+// Import specific version implementations for compatibility testing.
+#import "Sources/GULHeartbeatDateStorageV7.3.1.h"


Imports should be repo-relative - prepend with "GoogleUtilities"

paulb777 · 2021-05-10T22:47:47Z

GoogleUtilities/Tests/Unit/Environment/Sources/GULHeartbeatDateStorageV7.4.0.m

+ * limitations under the License.
+ */
+
+#import "GULHeartbeatDateStorageV7.4.0.h"


Make repo-relative

maksymmalyhin

LGTM on CI green, thank you for the fix and the tests!

Optionally we may add a couple more migration test cases e.g. test compatibility for the following: 7.3.1->7.4.0->7.4.1->7.3.1.

maksymmalyhin · 2021-05-10T22:46:45Z

GoogleUtilities/Tests/Unit/Environment/Sources/GULHeartbeatDateStorageV7.3.1.h

+#import <Foundation/Foundation.h>
+
+NS_ASSUME_NONNULL_BEGIN
+


optional: I would add a comment in this and other similar files that this is a copy of the specific file version required for compatibility tests.

I'll do this!

This reverts commit 44110e6.

…o nc/fix-hb-bug

ncooke3 added 3 commits May 6, 2021 21:26

Unsuccessful repro attempt

b9fb016

Reproduced 8047 crash

5ec9c40

Fix hb issue related to 7.4 release

c633300

Edit tests

c679faf

ncooke3 mentioned this pull request May 7, 2021

setHeartbeatDate:forTag: API causes crashes when stored heartbeat dictionary is immutable #36

Closed

ncooke3 requested review from maksymmalyhin, ryanwilson and paulb777 May 7, 2021 20:28

ncooke3 added 2 commits May 7, 2021 16:33

Update CHANGELOG

9163cdb

Remove whitespace from CHANGELOG

1f51ba2

maksymmalyhin reviewed May 7, 2021

View reviewed changes

ncooke3 and others added 4 commits May 7, 2021 17:53

Review

6df9128

Review 2

5612e74

Update GULHeartbeatDateStorageTest.m

9714115

Update comment

bcae310

maksymmalyhin reviewed May 9, 2021

View reviewed changes

Add tests for unexpected types

637e4ff

paulb777 reviewed May 10, 2021

View reviewed changes

Update podspec

c6e09e6

ncooke3 commented May 10, 2021

View reviewed changes

maksymmalyhin reviewed May 10, 2021

View reviewed changes

ncooke3 and others added 3 commits May 10, 2021 12:36

Date validation

40b5b1a

Add tests for issue fix and type validation

d8ceabe

Use bracket syntax

2acab96

ncooke3 commented May 10, 2021

View reviewed changes

Add compatibility tests

2c8dce4

maksymmalyhin reviewed May 10, 2021

View reviewed changes

Add compatibility tests revision

67160a1

paulb777 reviewed May 10, 2021

View reviewed changes

maksymmalyhin approved these changes May 10, 2021

View reviewed changes

ncooke3 and others added 8 commits May 10, 2021 18:57

Fix CI

357e769

Fix CI 2

b298077

Fix CI 3

2706c5a

Re-add files through Xcode UI

44110e6

Revert "Re-add files through Xcode UI"

91a999b

This reverts commit 44110e6.

Add new files to ARC files list in the test spec

dd776bc

Uncomment validation failed due to disabled ARC

ee9bdbb

Merge branch 'nc/fix-hb-bug' of github.com:google/GoogleUtilities int…

b4733d2

…o nc/fix-hb-bug

maksymmalyhin merged commit 5835941 into main May 11, 2021

maksymmalyhin deleted the nc/fix-hb-bug branch May 11, 2021 00:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crashes where `setHeartbeatDate:forTag:` reads immutable dictionary. #37

Fix crashes where `setHeartbeatDate:forTag:` reads immutable dictionary. #37

ncooke3 commented May 7, 2021 •

edited

google-oss-bot commented May 7, 2021 •

edited

`GoogleUtilities-ios-unit-GoogleUtilities.framework`

maksymmalyhin left a comment

maksymmalyhin May 7, 2021

paulb777 May 7, 2021

maksymmalyhin May 7, 2021

maksymmalyhin left a comment

paulb777 left a comment

ncooke3 May 10, 2021

maksymmalyhin May 10, 2021 •

edited

ncooke3 May 10, 2021 •

edited

maksymmalyhin May 10, 2021

paulb777 May 10, 2021

paulb777 May 10, 2021

maksymmalyhin left a comment •

edited

maksymmalyhin May 10, 2021

ncooke3 May 10, 2021

		#pragma mark - GoogleUtilities Issue #36

		- (void)testFix_pre7dot4_CompabitibilityIssue {

Fix crashes where setHeartbeatDate:forTag: reads immutable dictionary. #37

Fix crashes where setHeartbeatDate:forTag: reads immutable dictionary. #37

Conversation

ncooke3 commented May 7, 2021 • edited

google-oss-bot commented May 7, 2021 • edited

Coverage Report

Affected SDKs

GoogleUtilities-ios-unit-GoogleUtilities.framework

Test Logs

maksymmalyhin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maksymmalyhin left a comment

Choose a reason for hiding this comment

paulb777 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maksymmalyhin May 10, 2021 • edited

Choose a reason for hiding this comment

ncooke3 May 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maksymmalyhin left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix crashes where `setHeartbeatDate:forTag:` reads immutable dictionary. #37

Fix crashes where `setHeartbeatDate:forTag:` reads immutable dictionary. #37

ncooke3 commented May 7, 2021 •

edited

google-oss-bot commented May 7, 2021 •

edited

`GoogleUtilities-ios-unit-GoogleUtilities.framework`

maksymmalyhin May 10, 2021 •

edited

ncooke3 May 10, 2021 •

edited

maksymmalyhin left a comment •

edited