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

math/big: add Int.FillBytes #35833

Open
FiloSottile opened this issue Nov 25, 2019 · 11 comments
Open

math/big: add Int.FillBytes #35833

FiloSottile opened this issue Nov 25, 2019 · 11 comments
Assignees
Projects
Milestone

Comments

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Nov 25, 2019

When implementing cryptography, what we need is almost always a fixed size buffer representing a value in big endian. What math/big.Int.Bytes provides is a variable size buffer, so all over the place there are snippets of code that do make, Bytes and copy with a len dependent index. I just implemented one the other day for https://golang.org/cl/208484, and just saw one in x/crypto/acme.

I'd be willing to bet that every Bytes invocation in a crypto package is doing something similar. I also learned that random bugs that occur approximately every 256 executions are probably due to missing this step, and I have found such a bug in the wild at least twice.

I propose we solve this at the math/big API level, and add (*math/big.Int).BytesWithSize.

// BytesWithSize returns the absolute value of x as a big-endian
// byte slice of length size. If x doesn't fit in such a buffer,
// BytesWithSize will panic.
func (x *Int) BytesWithSize(size int) []byte {
	b := x.Bytes()
	switch {
	case len(b) > size:
		panic("math/big: value won't fit requested size")
	case len(b) == size:
		return b
	default:
		buf := make([]byte, size)
		copy(buf[size-len(b):], b)
		return buf
	}
}

I don't have a strong opinion on the len(b) > size behavior. Where we use it in crypto packages we always know an upper bound, and if we cross it we might as well panic because something went catastrophically wrong. (And the current code would either panic or silently truncate, which is worse.)

/cc @griesemer @katiehockman

@gopherbot gopherbot added this to the Proposal milestone Nov 25, 2019
@gopherbot gopherbot added the Proposal label Nov 25, 2019
@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Nov 25, 2019

Would it make sense to provide the byte slice to avoid a copy?

func (x *Int) AsBytes(buf []byte)
@robpike

This comment has been minimized.

Copy link
Contributor

@robpike robpike commented Nov 26, 2019

Yes, I like @griesemer's design. Panic is OK if documented. A nil argument could allocate, perhaps?
That is, if you provide a buffer it must be big enough; otherwise one will be allocated.

@FiloSottile

This comment has been minimized.

Copy link
Member Author

@FiloSottile FiloSottile commented Nov 26, 2019

Oh, I like that!

The nil argument behavior would make sense if we didn't have Bytes, but that's there to stay.

I am not a fan of the fact that one can't really tell which is which based on the names (Bytes vs. AsBytes) but I don't have better suggestions. Maybe BytesWithBuffer? It's a mouthful but hopefully clear? WriteBytes?

@magical

This comment has been minimized.

Copy link
Contributor

@magical magical commented Nov 26, 2019

ToFixedBytes? Since the difference from Bytes is that it writes to a fixed-size array (with left padding).

WriteBytes?

The usual name for this would be Read, not Write 😉

@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Nov 26, 2019

My original thought was also Read, but that name suggests that there's a result (n int, err error) as well which is perhaps overkill, especially if we never expect an error and if we don't care about the n since the buffer will be 0-padded. Also, given a Read method, one would expect a client to be able to call Read repeatedly to get all the bytes of an Int, which is not the case.

Maybe FillBytes if you don't like AsBytes? The former implies more of an "action" (of filling in the bytes) rather than a function call (that returns a value) as does the latter.

@FiloSottile

This comment has been minimized.

Copy link
Member Author

@FiloSottile FiloSottile commented Dec 2, 2019

I like FillBytes, thank you!

// FillBytes sets buf to the absolute value of x in big-endian, zero
// padded as necessary. If x doesn't fit in buf, FillBytes will panic.
func (x *Int) FillBytes(buf []byte)
@robpike

This comment has been minimized.

Copy link
Contributor

@robpike robpike commented Dec 2, 2019

// FillBytes sets buf to the absolute value of x, storing it as a zero-extended
// big-endian byte stream. If the encoded value doesn't fit in buf, FillBytes will panic.
func (x *Int) FillBytes(buf []byte)
@robpike

This comment has been minimized.

Copy link
Contributor

@robpike robpike commented Dec 2, 2019

I'm a little unsettled by the absolute value thing, but that's what Bytes does.

@FiloSottile

This comment has been minimized.

Copy link
Member Author

@FiloSottile FiloSottile commented Dec 3, 2019

I'm a little unsettled by the absolute value thing, but that's what Bytes does.

Same, but on the other hand every way of encoding negative numbers I know of went terribly wrong, so I see where that choice is coming from.

@rsc rsc added this to Incoming in Proposals Dec 4, 2019
@rsc rsc changed the title proposal: math/big: add Int.BytesWithSize proposal: math/big: add Int.FillBytes Dec 11, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 11, 2019

Based on the discussion, this sounds like a likely accept.

Leaving open for a week for final comments.

@rsc rsc moved this from Incoming to Likely Accept in Proposals Dec 11, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jan 8, 2020

No final comments, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Jan 8, 2020
@rsc rsc changed the title proposal: math/big: add Int.FillBytes math/big: add Int.FillBytes Jan 8, 2020
@rsc rsc modified the milestones: Proposal, Backlog Jan 8, 2020
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.15 Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.