Reflecting on byteslib behavior

[claudijd]

Description

./mod/primitives/bytes/bytes.go suffers from similar silent truncation behavior as in #796. The following functions (ToBytes32, ToBytes48, ToBytes96) all accept an arbitrary []byte and silently truncate as the comments suggest. However, I'm not sure this is ideal behavior. I believe this would open beacon-kit to a class of bugs where a parsing operation using one of these functions supplies an oversized slice that results in a silent truncation.

I believe that a simple invariant check in the methods themselves or in ExtendToSize would better prevent the beacon kit from being subject to this bug class over the project's life cycle and avoid misuse. I understand why this behavior is being used; writing without error-checking/handling is certainly easier. However, when dealing with byte parsing, I think being more explicit saves you from this bug class, resulting in undetected error cases.

This invariant check should ensure that len(input) is <= [N]byte.

Additionally, the behavior of ExtendToSize and PrependExtendToSize may also be something to reconsider to avoid the exact opposite problem, where you specify a length expectation shorter than the slice being passed in and get more than expected in return. In the current behavior, what happens when you say pass "FooBar" in with an index of 3? You get "FooBar" (a slightly new behavior) and not "Foo" (similar to the other functions above) or an invariant error (as I'm proposing for both cases).

In this case, I think inputs that are larger than the requested length should return an error.

Recommendation

Again, as said above, the current implementation of bytes is the easiest to use and doesn't require much error handling. However, if this error handling is not required, you run the risk of these bug classes remaining in the code base undetected when/if they do occur.

• Don't allow silent truncation behaviors to exist, by adding max length invariant checks for arbitrary []byte and causing an error, which must be handled.
• Don't allow silent reflection of oversized slice behaviors to exist by changing max length invariant checks when []byte input is larger than length input to return an error, which must be handled.

[itsdevbear]

Agreed will address. We may have to accept that it is better to have the dreaded err!=nil check, and lose the nicety of chaining calls.

beacon-kit-rs, one day 😭