Skip to content

Minimal fix for b/74357976. #890

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.

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

Merged

Conversation

mikelehen
Copy link
Contributor

This is a more minimal alternative to #887.

Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

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

Content LGTM, but I'm not sure this is the right branch. Do not merge.

@wilhuff wilhuff requested a review from paulb777 March 8, 2018 18:04
@wilhuff
Copy link
Contributor

wilhuff commented Mar 8, 2018

Oh also, changelog

Copy link
Contributor

@zxu123 zxu123 left a comment

Choose a reason for hiding this comment

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

LGTM thank you for helping this.

@mikelehen
Copy link
Contributor Author

Good call, added changelog entry.

@paulb777
Copy link
Member

paulb777 commented Mar 8, 2018

I made a release-4.10.1 branch. Please target it there.

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.

Change merge target to release-4.10.1

@mikelehen mikelehen changed the base branch from release-4.10.0 to release-4.10.1 March 8, 2018 18:56
@mikelehen
Copy link
Contributor Author

I've changed the target branch, but we're looking into an additional (perhaps related issue) so may want to hold off on merging this.

@mikelehen mikelehen changed the title Minimal fix for b/74357976 Minimal fix for b/74357976 and b/74357976 Mar 8, 2018
@mikelehen
Copy link
Contributor Author

Okay, I've incorporated the second fix that @wilhuff tracked down. PTAL

@mikelehen
Copy link
Contributor Author

Actually @wilhuff is doing a further audit of WrapNSStringNoCopy() usages for additional potential bugs...

…NSStringNoCopy() issue which @wilhuff is going to create a fix for.
@mikelehen mikelehen force-pushed the mikelehen/fix-unauthenticated-access-minimal branch from 37073b6 to 3b4fa32 Compare March 8, 2018 20:08
@mikelehen
Copy link
Contributor Author

@wilhuff is going to create a separate PR for all of the util:WrapNSStringNoCopy() fixes. This PR will just be for the original token issue and should be good-to-go now.

Copy link
Contributor

@morganchen12 morganchen12 left a comment

Choose a reason for hiding this comment

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

Approving on @paulb777's behalf, since this PR is now targeting the correct branch.

@morganchen12 morganchen12 dismissed paulb777’s stale review March 8, 2018 20:17

Paul briefly unavailable--see review comment

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.

Please fix duplicate in title

@wilhuff wilhuff changed the title Minimal fix for b/74357976 and b/74357976 Minimal fix for b/74357976 Mar 8, 2018
@mikelehen mikelehen changed the title Minimal fix for b/74357976 Minimal fix for b/74357976. Mar 8, 2018
@mikelehen mikelehen merged commit 622a911 into release-4.10.1 Mar 8, 2018
@mikelehen mikelehen deleted the mikelehen/fix-unauthenticated-access-minimal branch March 8, 2018 20:19
zxu123 added a commit that referenced this pull request Mar 8, 2018
zxu123 added a commit that referenced this pull request Mar 9, 2018
* Version bumps for Firebase 4.10.1 (#891)

* Minimal fix for b/74357976 (#890)

Fixes b/74357976 which caused unauthenticated users to be unable to reach the Firestore backend and updates the changelog.

* Copy all C++ strings to NSString where they're not obviously safe (#893)

This fixes a known instances of memory corruption where in
FSTLevelDBMutationQueue, the NSString view was retained for later, and
the incorrect user was used, causing b/74381054.

gRPC does not necessarily copy its string argumnets and if our hostname
were configured to a non-default one it's possible that we could corrupt
the host cache too.

All remaining usages of util::WrapNSStringNoCopy are obviously safe:
passed into logging or other known transient usages.

* fix lint
minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018
Fixes b/74357976 which caused unauthenticated users to be unable to reach the Firestore backend and updates the changelog.
minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018
* Version bumps for Firebase 4.10.1 (firebase#891)

* Minimal fix for b/74357976 (firebase#890)

Fixes b/74357976 which caused unauthenticated users to be unable to reach the Firestore backend and updates the changelog.

* Copy all C++ strings to NSString where they're not obviously safe (firebase#893)

This fixes a known instances of memory corruption where in
FSTLevelDBMutationQueue, the NSString view was retained for later, and
the incorrect user was used, causing b/74381054.

gRPC does not necessarily copy its string argumnets and if our hostname
were configured to a non-default one it's possible that we could corrupt
the host cache too.

All remaining usages of util::WrapNSStringNoCopy are obviously safe:
passed into logging or other known transient usages.

* fix lint
@firebase firebase locked and limited conversation to collaborators Nov 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants