Repository navigation
Conversation
Cached settings are read at launch while the crash reporter is being set up, so malformed data in the settings file or its cache key could crash on every launch. Validate the types of the settings dictionary, its groups, its values and the cache key fields; fall back to defaults for wrong-typed, negative or non-finite values; clamp values too large for uint32_t; and reject a zero on-demand backoff step duration, which is used as a divisor. Refs #16728
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of Crashlytics settings parsing by introducing value validation helpers to safely handle malformed or unexpected types from the server or disk cache, preventing potential startup crashes. The review feedback suggests replacing isEqualToString: with isEqual: when comparing cached values (such as Google App ID, build instance ID, and app version) to avoid crashes if the compared properties are nil.
…ey checks Addresses review feedback: compare cached Google App ID, build instance ID and app version with isEqual: instead of isEqualToString: so a nil current value is handled as a mismatch. Adds tests for nil build instance ID and nil Google App ID.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of Crashlytics settings parsing by introducing validation helpers to handle malformed, unexpected, or out-of-range values from the server or cache, accompanied by comprehensive unit tests. The review feedback suggests checking for NSNull in FIRCLSSettingsIsValidNumber to prevent redundant debug logging for explicit JSON nulls, and using doubleValue instead of unsignedIntValue when reading the cached creation timestamp to preserve full precision.
| id cacheCreatedAtValue = cacheKey[CreatedAtKey]; | ||
| NSTimeInterval cacheCreatedAt = [cacheCreatedAtValue isKindOfClass:[NSNumber class]] | ||
| ? [cacheCreatedAtValue unsignedIntValue] | ||
| : 0; |
There was a problem hiding this comment.
The CreatedAtKey is stored as a double representing the full NSTimeInterval timestamp in cacheSettingsWithGoogleAppID:currentTimestamp:. Retrieving it using unsignedIntValue truncates the fractional seconds. Using doubleValue preserves the full precision of the NSTimeInterval and is more idiomatic.
| id cacheCreatedAtValue = cacheKey[CreatedAtKey]; | |
| NSTimeInterval cacheCreatedAt = [cacheCreatedAtValue isKindOfClass:[NSNumber class]] | |
| ? [cacheCreatedAtValue unsignedIntValue] | |
| : 0; | |
| id cacheCreatedAtValue = cacheKey[CreatedAtKey]; | |
| NSTimeInterval cacheCreatedAt = [cacheCreatedAtValue isKindOfClass:[NSNumber class]] | |
| ? [cacheCreatedAtValue doubleValue] | |
| : 0; |
There was a problem hiding this comment.
Done in 084fe8f. created_at is now read via the existing FIRCLSSettingsDoubleValue helper. Missing, non-numeric, negative or non-finite values fall back to 0, which expires the cache. That also fixes a negative integer wrapping to a far-future timestamp under unsignedIntValue. Added tests for fractional seconds and for invalid values.
Addresses review feedback: - Read created_at back as a double (it is written as one) instead of truncating it with unsignedIntValue. Missing, non-numeric, negative or non-finite values fall back to 0 so the TTL check expires the cache; previously a negative integer wrapped to a far-future timestamp. - Treat explicit JSON nulls like missing values when validating numeric settings so they fall back to the default without a debug log.
ncooke3
left a comment
ncooke3
left a comment
There was a problem hiding this comment.
Non-blocking follow-ups: (1) consider falling back to the default instead of clamping out-of-range uint32_t values to UINT32_MAX, since a clamped cache_duration never expires and a clamped log_buffer_size effectively removes the log limit; (2) confirm the backend never sends on_demand_upload_rate_per_minute / on_demand_backoff_base as strings, since those are now ignored.
This branch has not been deployed
4 participants
Context: #16728 — a malformed server payload parsed into an unexpected type crashed
production apps. This PR hardens the equivalent path in Crashlytics: settings
fetched from
firebase-settings.crashlytics.com/spi/v2/.../settings, cached todisk, and re-read at every launch in
FIRCLSSettings.This matters more than usual for Crashlytics: cached settings are read while the
crash reporter is being set up (
FIRCLSContextBuildInitData), so a malformedcached settings file would crash on every launch until the cache expired or the
app was reinstalled, and Crashlytics would likely not report that crash.
Hardened code paths (all in
FIRCLSSettings.m)reloadFromCacheWithGoogleAppID:— settings file is valid JSON but not an object (e.g.[1,2,3])-[__NSArrayI objectForKeyedSubscript:]unrecognized selector (NSInvalidArgumentException). The file is now discarded like other corrupt settings files.app/session/features/fabricgroups that aren't objects-[NSTaggedPointerString objectForKeyedSubscript:]unrecognized selector. Now treated as missing.null(cache_duration,collect_*,log_buffer_size,max_custom_*,on_demand_*)-[NSNull unsignedIntValue]unrecognized selector. Now falls back to the default. Bool switches still acceptNSNumberorNSString, as before.uint32_tsettingsunsignedIntValue. For example,cache_duration: -1produced ~136 years, so the settings TTL never expired. Now falls back to the default.uint32_t4294967296became0. Now clamped toUINT32_MAX.on_demand_upload_rate_per_minute,on_demand_backoff_base(-1e400parses as-inf)pow, division). Now falls back to the default.on_demand_backoff_step_duration_secondsof0(or a value that truncates/wraps to 0)FIRCLSOnDemandModel→ NaN step/delay. Now falls back to the default (6).NSJSONReadingAllowFragments)-[__NSCFString objectForKeyedSubscript:]unrecognized selector. Now treated as a missing cache key, which invalidates the cache.google_app_id,created_at,build_instance_id,app_version)-[__NSCFNumber isEqualToString:]unrecognized selector. Now treated as a mismatch (Google App ID → cache deleted; others → cache marked expired).Behavior for valid payloads does not change. Zero is still respected wherever
it means something (e.g.
log_buffer_size: 0,on_demand_upload_rate_per_minute: 0,cache_duration: 0).Tests
Added to
FIRCLSSettingsTests:testSettingsNotADictionarytestSettingsGroupsWithWrongTypestestSettingsValuesWithWrongTypestestSettingsNegativeValuestestSettingsOutOfRangeValuestestSettingsZeroAndFractionalValuestestCacheKeyNotADictionarytestCacheKeyGoogleAppIDWrongTypetestCacheKeyValuesWithWrongTypesI ran each one on
mainwithout the fix. All 9 failed. Six of them threwNSInvalidArgumentException(unrecognized selector):testSettingsNotADictionary,testSettingsGroupsWithWrongTypes,testSettingsValuesWithWrongTypes,testCacheKeyNotADictionary,testCacheKeyGoogleAppIDWrongTypeandtestCacheKeyValuesWithWrongTypes. The other 3 failed on assertions (wrapped ortruncated values, and a zero step duration). With the fix, all 9 pass.
Testing
xcodebuild -scheme FirebaseCrashlyticsUnit -destination 'platform=macOS' test(SPM): full
FirebaseCrashlyticsUnitsuite passes.