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.

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

Fix sharing coded buffer bytes when parsing bytes fields #640

Merged
merged 1 commit into from
May 4, 2022

Conversation

osa1
Copy link
Member

@osa1 osa1 commented May 4, 2022

Synced from internal repo


I'm not sure if this is the right fix for this bug. Should readBytes() return a new list (instead of a view) maybe?

@osa1 osa1 requested a review from sigurdm May 4, 2022 12:37
Copy link
Collaborator

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

LGTM

@osa1
Copy link
Member Author

osa1 commented May 4, 2022

I'm not sure if this is the right fix for this bug. Should readBytes() return a new list (instead of a view) maybe?

We discussed this fix with @sigurdm. Some notes about
CodedBufferReader.readBytes:

  • It's supposed to be internal, but it's currently part of the public API. This
    is probably accidental.

  • In the library we have 4 uses of this method, and with this PR all of them
    clone the returned list, either explicitly with Uint8List.fromList, or by
    decoding the returned list to a string, or appending returned list to another
    list.

  • Internally (in g3) we have a few users, they all explicitly copy the returned
    list.

  • So in principle we could make CodedBufferReader.readBytes return a new list
    (instead of a view).

  • It's also not unreasonable to make proto messages share the buffer, but we
    need to do one of the following:

    • Document it so that the users will clone the buffer to avoid sharing (note
      that proto messages are not length prefixed, and I don't know how difficult
      or inconvenient this would be in practice)
    • Implement copy-on-write so that backing buffer will never be modified

@osa1 osa1 merged commit e30a522 into google:master May 4, 2022
@osa1 osa1 deleted the internal_2 branch May 4, 2022 13:18
osa1 added a commit to osa1/protobuf.dart that referenced this pull request May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants