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

proposal: bytes: trim cap on Buffer.Bytes result #59570

Closed
dolmen opened this issue Apr 12, 2023 · 10 comments
Closed

proposal: bytes: trim cap on Buffer.Bytes result #59570

dolmen opened this issue Apr 12, 2023 · 10 comments
Labels
Proposal
Milestone
Proposal

Comments

@dolmen
Copy link
Contributor

dolmen commented Apr 12, 2023

The current implementation of bytes.Buffer.Bytes() is:

func (b *Buffer) Bytes() []byte { return b.buf[b.off:] }

In this implementation the capacity of the returned slice is the full capacity of internal storage of the Buffer. This has the following issues:

  1. Information about internal state is revealed through something which is not a official API (information leakage). See https://go.dev/play/p/Li5MsayY7_F. If the slice is passed through libraries, this leakage could go through multiple layers (contrary to a call to Buffer.Cap with requires direct access to the Buffer object)
  2. In the case of misuse of the Bytes() result (I know this a flawed assumption), an append could write to Buffer's internal storage (but I don't see a way it could be exploited besides the fact that this internal storage might be he initial slice passed to bytes.NewBuffer which might be shared somewhere else).

I propose to limit the capactity of the returned slice to the strict size of the data returned. I think that this could also reduce exploit vectors in the (flawed) case of a bad use of the Buffer API (which might happen unexpectedly if the result of Bytes() is passed without care through multiple layers of libraries).

Proposed implementation:

func (b *Buffer) Bytes() []byte { return b.buf[b.off:len(b.buf):len(b.buf)] }

A similar change would also be applied to Buffer.Next(int).
@gopherbot gopherbot added this to the Proposal milestone Apr 12, 2023
@dolmen
Copy link
Contributor Author

dolmen commented Apr 12, 2023

#42986 also raised the capacity issue.

@seankhliao
Copy link
Member

The result of Bytes() is already documented as only valid until the next modification.
Even with the proposed change, it's still an issue if you hold a slice to the exposed portion, but the buffer is then Reset(), making your view into what has now become buffer's internal state.

@dolmen
Copy link
Contributor Author

dolmen commented Apr 12, 2023

@seankhliao I didn't say otherwise. What you are writing is already clearly marked in the documentation and I have acknowledged that ("I know this a flawed assumption").

I didn't claim either to fix code which uses bytes.Buffer incorrectly.

I just think it would be wise to mitigate some of the side-effects of that kind of broken code.

@dsnet
Copy link
Member

dsnet commented Apr 12, 2023

I'm generally in favor of this semantic, but I don't see how we can change this today without breaking existing programs.

Before the addition of bytes.Buffer.AvailableBuffer, the unused capacity had been abused as a way to directly append to the unwritten portion of the buffer.

@dolmen
Copy link
Contributor Author

dolmen commented Apr 13, 2023

@dsnet I can't find bytes.Buffer.AvailableBuffer. Are you refering to bytes.Buffer.Cap()?

@dsnet
Copy link
Member

dsnet commented Apr 13, 2023

See #53685.

@dolmen
Copy link
Contributor Author

dolmen commented Apr 13, 2023

Ah, bufio.Writer.AvailableBuffer in #47527.

@rsc
Copy link
Contributor

rsc commented May 10, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc changed the title proposal: bytes: Buffer.Bytes(): set strict capacity proposal: bytes: trim cap on Buffer.Bytes result May 10, 2023
@rsc
Copy link
Contributor

rsc commented May 10, 2023

I agree that this seems infeasible.

@dolmen dolmen closed this as completed May 11, 2023
@rsc
Copy link
Contributor

rsc commented May 17, 2023

This proposal has been declined as retracted.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal
Projects
Proposals
Status: Declined
Milestone
Proposal
Development

No branches or pull requests
5 participants
@rsc @dolmen @dsnet @gopherbot @seankhliao