Fix crash caused by ResponseJson::MarkCompleted() failing to set application_data_ if parsing the JSON body fails by dconeybe · Pull Request #692 · firebase/firebase-cpp-sdk

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