E548 Fix heap-buffer overflow in FPREncodePerfSessions by jeremyjiang-dev · Pull Request #8872 · firebase/firebase-ios-sdk · GitHub
[go: up one dir, main page]
Skip to content

Fix heap-buffer overflow in FPREncodePerfSessions #8872

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
Merged
merged 9 commits into from
Nov 4, 2021
Merged

Fix heap-buffer overflow in FPREncodePerfSessions #8872

merged 9 commits into from
Nov 4, 2021

Conversation

jeremyjiang-dev
Copy link
Contributor

Fix #8849.

@google-cla google-cla bot added the cla: yes label Oct 27, 2021
@jeremyjiang-dev
Copy link
Contributor Author

@paulb777 Is it possible to enable Address Sanitizer when running tests in CI so that we can catch these issues early?

paulb777
paulb777 reviewed Oct 27, 2021
View reviewed changes
Copy link
Member
@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

Seems like a definite improvement. It's problematic to calloc a zero length array. :)

Where does the session_verbosity array expand and how is the memory expanded.

Does this even need to be dynamic memory if there is only ever one element in it?

@paulb777
Copy link
Member

See the code in build.sh that Firestore uses for running sanitizers. It should be adaptable for Performance

@google-oss-bot
Copy link
google-oss-bot commented Oct 27, 2021 •
edited
Loading

Coverage Report

Affected SDKs

  • FirebasePerformance-iOS-FirebasePerformance.framework

    SDK overall coverage changed from 91.86% (aec1dd4) to 91.79% (fc03b36) by -0.07%.

    Filename Base (aec1dd4) Head (fc03b36) Diff
    FPRConfigurations.m 93.27% 91.99% -1.28%

Test Logs

@visumickey
Copy link
Contributor

The changes look good to me here. But, we might want to do the static analyzer checks as well as a part of this PR to make sure we catch these issues ahead of time.

@jeremyjiang-dev
Copy link
Contributor Author

Seems like a definite improvement. It's problematic to calloc a zero length array. :)

Where does the session_verbosity array expand and how is the memory expanded.

Does this even need to be dynamic memory if there is only ever one element in it?

Seems like a definite improvement. It's problematic to calloc a zero length array. :)

Where does the session_verbosity array expand and how is the memory expanded.

Does this even need to be dynamic memory if there is only ever one element in it?

Good questions! For now, the session_verbosity has just 1 element. But overtime, we would have more elements there.
However, nanoPb needs to know the array length before calloc memory for arrays. I would prefer calloc an array of size 1. Let me know if you have any suggestions. @visumickey

@paulb777
Copy link
Member

Thanks for the explanation @jeremyjiang-dev . If the array could expand in the future, then doesn't session_verbosity_count need to part of the protocol? Otherwise, it looks like the code would need to be rewritten anyway when there is a second element and session_verbosity could be a pointer instead of an array for now?

@jeremyjiang-dev
Copy link
Contributor Author

Thanks for the explanation @jeremyjiang-dev . If the array could expand in the future, then doesn't session_verbosity_count need to part of the protocol? Otherwise, it looks like the code would need to be rewritten anyway when there is a second element and session_verbosity could be a pointer instead of an array for now?

I agree that the code would need to be rewritten anyway. Without knowing what might be added to the session_verbosity array, it is hard to account for future expansion. Unlike an GPBEnumArray in the previous implementation, which could add elements dynamically, I need to know the session_verbosity_count before allocating memory. @visumickey Could you clarify what need to be added to the array?

@visumickey
Copy link
Contributor

Thanks for the explanation @jeremyjiang-dev . If the array could expand in the future, then doesn't session_verbosity_count need to part of the protocol? Otherwise, it looks like the code would need to be rewritten anyway when there is a second element and session_verbosity could be a pointer instead of an array for now?

@paulb777 I'm not sure of your suggestion here. The current proto field for the session_verbosity is a repeated field and that our representation of the field as an Array. Since this is an array of Ints, we are bound to do allocation based on the count and use it. I think this is also future proof and should not be any concerning.

Can you qualify what I'm missing here?

@paulb777
Copy link
Member
paulb777 commented Nov 2, 2021

Maybe I'm missing something but doesn't the line perfSessions[perfSessionIndex].session_verbosity_count = 1; hardcode the array count to 1, so the code will fail if the server ever starts sending more than 1?

@visumickey
Copy link
Contributor

Maybe I'm missing something but doesn't the line perfSessions[perfSessionIndex].session_verbosity_count = 1; hardcode the array count to 1, so the code will fail if the server ever starts sending more than 1?

Ha, get it now. Unfortunately this array would have to be hard coded to 1 because of how the proto is defined. The array will expand based on the number of allowed verbosity qualifiers. For now, this would be hardcoded to just 1.

paulb777
paulb777 approved these changes Nov 3, 2021
View reviewed changes
Copy link
Member
@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

LGTM on green CI.

Maybe the failing sanitizer checks should be deferred to another PR?

@jeremyjiang-dev
Copy link
Contributor Author

LGTM on green CI.

Maybe the failing sanitizer checks should be deferred to another PR?

Thanks Paul! I will add sanitizer checks in another PR.

@jeremyjiang-dev jeremyjiang-dev merged commit ec9309d into master Nov 4, 2021
@jeremyjiang-dev jeremyjiang-dev deleted the perfHeapBuffer branch November 4, 2021 15:26
@paulb777 paulb777 mentioned this pull request Nov 9, 2021
Closed
@firebase firebase locked and limited conversation to collaborators Dec 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@paulb777 paulb777 paulb777 approved these changes

@visumickey visumickey Awaiting requested review from visumickey

Assignees
No one assigned
Labels
api: performance cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FPREncodePerfSessions causes heap-buffer overflow
4 participants
@jeremyjiang-dev @paulb777 @google-oss-bot @visumickey
0