Skip to content
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

Jump to bottom

Fix crash caused by ResponseJson::MarkCompleted() failing to set application_data_ if parsing the JSON body fails #692

Merged
merged 4 commits into from
Sep 30, 2021
Merged

Fix crash caused by ResponseJson::MarkCompleted() failing to set application_data_ if parsing the JSON body fails #692

merged 4 commits into from
Sep 30, 2021

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Sep 29, 2021 •
edited
Loading

Description

When parsing the JSON response body in ResponseJson::MarkCompleted(), if parsing failed then, if assertions were disabled, the method would return without setting the application_data_ member to a valid value. Then, later on, if a method like AuthResponse::error_code() is invoked then it would access application_data_ assuming it was valid. If, however, MarkCompleted() failed to initialize it then it would not be valid and result in a crash, such as EXC_BAD_ACCESS.

The problem was that the success of the JSON parsing was being "asserted" by calling FIREBASE_ASSERT_RETURN_VOID. So if assertions did not cause a crash then MarkCompleted() would return without setting application_data_ or calling the superclass' MarkCompleted() method. As you can see from earlier in the method when it checks for a zero-length response body, if such an error occurs then it should do both of these things.

The fix in this PR is to replace FIREBASE_ASSERT_RETURN_VOID with LogError() and make sure to perform these two critical steps before returning. We decided that asserting on the correctness of external data over which the app does not have control is inappropriate, and it is better to log the error and carry on.

Testing

This fix was tested by creating a custom build and having the customer who reported the issue (#615) test out the fix.

Type of Change

Place an x the applicable box:

  • Bug fix. Add the issue # below if applicable.
  • New feature. A non-breaking change which adds functionality.
  • Other, such as a build process or documentation change.

Fixes #615
@dconeybe
Fix a crash caused by ResponseJson::MarkCompleted() failing to set ap…
167c05f 
…plication_data_ if parsing the JSON body fails
@dconeybe dconeybe self-assigned this Sep 29, 2021
@google-cla google-cla bot added the cla: yes label Sep 29, 2021
@dconeybe dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Sep 29, 2021
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Sep 29, 2021
@github-actions
Copy link

github-actions bot commented Sep 29, 2021 •
edited
Loading

❌  Integration test FAILED

Requested by @dconeybe on commit ef28f10
Last updated: Thu Sep 30 10:20 PDT 2021
View integration test log & download artifacts

Failures Configs
admob [BUILD] [ERROR] [Android] [macos]
analytics [BUILD] [ERROR] [Android] [macos]
auth [BUILD] [ERROR] [Android] [macos]
database [BUILD] [ERROR] [Android] [macos]
dynamic_links [BUILD] [ERROR] [Android] [macos]
firestore [BUILD] [ERROR] [Android] [macos]
functions [BUILD] [ERROR] [Android] [macos]
installations [BUILD] [ERROR] [Android] [macos]
messaging [BUILD] [ERROR] [Android] [macos]
[TEST] [ERROR] [Android] [windows] [android_target]
remote_config [BUILD] [ERROR] [Android] [macos]

Add flaky tests to go/fpl-cpp-flake-tracker
@dconeybe dconeybe mentioned this pull request Sep 29, 2021
Closed
@dconeybe dconeybe assigned jonsimantov and unassigned dconeybe Sep 29, 2021
jonsimantov
jonsimantov requested changes Sep 29, 2021
View reviewed changes
app/rest/response_json.h Outdated
@@ -74,12 +74,26 @@ class ResponseJson : public Response {
// Parse and verify JSON string in body. FlatBuffer parser does not support
// online parsing. So we only parse the body when we get everything.
bool parse_status = parser_->Parse(GetBody());
FIREBASE_ASSERT_RETURN_VOID(parse_status);
FIREBASE_ASSERT_MESSAGE(parse_status,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With FIREBASE_ASSERT_MESSAGE, it will assert in C++ and throw an exception in Unity. Is that the behavior we want? Maybe this should just be a LogError. Same below.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if LogError() would be more appropriate. Assert in C++ and throw in Unity sounds reasonable though.

Note that both FIREBASE_ASSERT_RETURN_VOID and FIREBASE_ASSERT_MESSAGE use firebase::LogAssert() under the hood to log the assertion and crash/throw/ignore the failed assertion. So the change made in this PR does not change that behavior. If that should be changed then I think that should be a separate PR since I don't know the right answer and only learned enough about this Auth code to fix the bug at hand.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, what I'm saying is, asserting at all (in C++) on bad data coming in (outside the app's control) seems like a bad idea, no?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree. It's an inappropriate use of assertions since the data is out of the app's control. I guess the rationale is that using assertions makes it VERY visible what's going wrong. Otherwise, if we change it to just LogError(), it would be more difficult to correlate the ultimate failure with the root cause of a parsing failure. But I guess the fact that checking for a zero-length body earlier in the method and not even logging anything gives merit to the argument that we shouldn't crash. Ok, I'll change it to LogError().
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - yeah this fulfills the software robustness principle: "Be conservative in what you do, be liberal in what you accept from others."
@dconeybe dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Sep 29, 2021
@github-actions github-actions bot removed the tests-requested: quick Trigger a quick set of integration tests. label Sep 29, 2021
jonsimantov
jonsimantov previously approved these changes Sep 29, 2021
View reviewed changes
@dconeybe dconeybe enabled auto-merge (squash) September 29, 2021 19:05
DellaBitta
DellaBitta reviewed Sep 29, 2021
View reviewed changes
release_build_files/readme.md
@@ -576,6 +576,9 @@ code.
- Messaging (Android): Fixes an issue to receive token when
initialize the app.
([#667](https://github.com/firebase/firebase-cpp-sdk/pull/667)).
- Auth (Desktop): Fix a crash that would occur if parsing the JSON
response from the server failed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: period.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The period occurs after the PR link on line 581.
DellaBitta
DellaBitta previously approved these changes Sep 29, 2021
View reviewed changes
@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Sep 29, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Sep 29, 2021
@github-actions github-actions bot dismissed stale reviews from jonsimantov and DellaBitta September 30, 2021 00:53

🍞 Dismissed stale approval on external PR.

@dconeybe dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Sep 30, 2021
@github-actions github-actions bot added the tests: in-progress This PR's integration tests are in progress. label Sep 30, 2021
@github-actions github-actions bot added tests: succeeded This PR's integration tests succeeded. and removed tests-requested: quick Trigger a quick set of integration tests. tests: failed This PR's integration tests failed. labels Sep 30, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Sep 30, 2021
@dconeybe dconeybe merged commit ef28f10 into main Sep 30, 2021
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests: succeeded This PR's integration tests succeeded. labels Sep 30, 2021
@dconeybe dconeybe deleted the dconeybe/ResponseJsonMarkCompletedCrashFix branch September 30, 2021 15:00
@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Sep 30, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Sep 30, 2021
@firebase firebase locked and limited conversation to collaborators Oct 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: auth api: firestore cla: yes tests: failed This PR's integration tests failed.
3 participants
@dconeybe @jonsimantov @DellaBitta