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: add testcast for PutInteger #1277

Closed
wants to merge 3 commits into from

Conversation

atgane
Copy link
Contributor

@atgane atgane commented Dec 15, 2023

I've implemented three parts to catch errors.

  1. Receive an error if the len(data) is less than integer.size.
  2. if integer.Encoding is not signed, throw error if it exceeds the maximum value of the unsigned size.
  3. if integer.Encoding is signed, throw error when n is in a range that cannot be converted from uint64.

image

For example, the red part below throws an error.

Fixes #1208

@atgane atgane requested a review from a team as a code owner December 15, 2023 14:23
Signed-off-by: atgane <hyper201286@gmail.com>
@@ -263,6 +263,21 @@ func PutInteger(data []byte, integer *btf.Int, n uint64) error {
return fmt.Errorf("invalid boolean value: %d", n)
}

if len(data) < int(integer.Size) {
return fmt.Errorf("byte array size %d is less than the integer size %d", len(data), integer.Size)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: technically, data is a byte slice, not an array. Also, it's always better to tell the user what the problem is. Sure the byte slice is smaller, but why is that an issue?

Suggested change
return fmt.Errorf("byte array size %d is less than the integer size %d", len(data), integer.Size)
return fmt.Errorf("can't fit an integer of size %d into a byte slice of length %d", integer.Size, len(data))

lowerBound = lowerBound - upperBound - 1
}

if !(n <= upperBound || lowerBound < n) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is hard to read. Try:

Suggested change
if !(n <= upperBound || lowerBound < n) {
if n > upperBound || n < lowerBound {

}

upperBound := uint64(1<<(8*integer.Size) - 1)
lowerBound := uint64(math.MaxUint64)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this called lowerBound if the value used is math.MaxUint64? Wouldn't this typically be used as an upper bound? Also, there's no need to perform this check on unsigned ints if we enforce some kind of minimum length of data.

if !(n <= upperBound || lowerBound < n) {
return fmt.Errorf("%d exceeded the given byte range %d bytes size", n, integer.Size)
}

switch integer.Size {
case 1:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of the upperBound/lowerBound stuff from above, what about something like:

Suggested change
case 1:
case 1:
if integer.Encoding == btf.Signed && (n > math.MaxInt8 || n < math.MinInt8) {
return fmt.Errorf("...")
}

This makes it stupidly obvious what the intention is and avoids the subtle math. Go defines all these constants, so we should just make use of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks good, but math.MinInt8 is not unsigned value, so condition can be

(n > math.MaxInt8 && n < math.MaxUint64 - math.MaxInt8)

And I'll change to return the error message only if it's signed conditions.

Signed-off-by: atgane <hyper201286@gmail.com>
@atgane atgane closed this Jan 16, 2024
@atgane atgane deleted the feature/put-integer branch January 16, 2024 12:46
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.

kconfig: validate that PutInteger doesn't truncate data
2 participants