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

assert: EqualValues does not check for overflows #1462

Closed
vispariana opened this issue Aug 25, 2023 · 5 comments · Fixed by #1531
Closed

assert: EqualValues does not check for overflows #1462

vispariana opened this issue Aug 25, 2023 · 5 comments · Fixed by #1531
Assignees
Labels
bug help wanted pkg-assert Change related to package testify/assert

Comments

@vispariana
Copy link

vispariana commented Aug 25, 2023

When asserting two numeric values with EqualValues, it passes if one is the overflowed value of the other.

example: https://go.dev/play/p/KZsFh7FnBVc

func TestEqualValues(t *testing.T) {
	var a int8 = 14
	var b int = 270
	assert.Equal(t, b, a)       // fails because of type mismatch
	assert.EqualValues(t, b, a) // passes
}
@tscales
Copy link
Contributor

tscales commented Aug 29, 2023

if expectedValue.IsValid() && expectedValue.Type().ConvertibleTo(actualType) {

Go Allows for Ints to be converted to Int8s, hence the ConvertibleTo method returns true. A check would probably needed to be added that the Value of Expected > maxSize of the actual type before doing the DeepEqual call.

@vispariana
Copy link
Author

vispariana commented Aug 30, 2023

if expectedValue.IsValid() && expectedValue.Type().ConvertibleTo(actualType) {

Go Allows for Ints to be converted to Int8s, hence the ConvertibleTo method returns true. A check would probably needed to be added that the Value of Expected > maxSize of the actual type before doing the DeepEqual call.

How about checking it both ways instead of looking at max size?

assume we have var a A and var b B. if A is convertible to B then B is also convertible to A. So maybe we can just check if:
B(a) == b && A(b) == a

@dolmen dolmen added pkg-assert Change related to package testify/assert bug help wanted labels Oct 15, 2023
@dolmen dolmen changed the title [Bug] EqualValues does not check for overflows assert: EqualValues does not check for overflows Oct 15, 2023
@tscales
Copy link
Contributor

tscales commented Nov 8, 2023

assume we have var a A and var b B. if A is convertible to B then B is also convertible to A. So maybe we can just check if: B(a) == b && A(b) == a

var b can't be assumed convertible in all cases. For example: when a is an int and b is a string, the assertion would panic if we tried to introduce actualValue.Convert(ExpectedType)

https://go.dev/play/p/BKML-vPu4l_N
https://go.dev/play/p/eOi-PI8CLhk

@vispariana
Copy link
Author

vispariana commented Nov 9, 2023

I didn't know that conversion existed. Thank you!
I still believe that's the way to go though. If a and b are not convertible to each other, they are not "EqualValues". That's testify/assert's the current approach, because no order is implied on the two parameters, which leads to inconsistencies like this:

https://go.dev/play/p/XaCM39GdJum

So I think we'd be fine with adding a reverse convertibility check to prevent the panic you mentioned.

arjunmahishi added a commit to arjunmahishi/testify that referenced this issue Feb 10, 2024
The underlying function ObjectsAreEqualValues did not handle
overflow/underflow of values while converting one type to another
for comparison. For example:

    EqualValues(t, int(270), int8(14))

would return true, even though the values are not equal. Because, when
you convert int(270) to int8, it overflows and becomes 14 (270 % 256 = 14)

This commit fixes that by making sure that the conversion always happens
from the smaller type to the larger type, and then comparing the values.
Additionally, this commit also seperates out the test cases of
ObjectsAreEqualValues from TestObjectsAreEqual.

Fixes stretchr#1462
@arjunmahishi
Copy link
Collaborator

So I think we'd be fine with adding a reverse convertibility check

I don't think this will work. int8.ConvertibleTo(int) and int.ConvertibleTo(int8) both return true. I've raised a PR with a simple fix. We just need to ensure that we are converting smaller type to a larger type (int8 to int in this case). In the EqualValues function, the order of the arguments should not matter anyway.

arjunmahishi added a commit to arjunmahishi/testify that referenced this issue Feb 12, 2024
The underlying function ObjectsAreEqualValues did not handle
overflow/underflow of values while converting one type to another
for comparison. For example:

    EqualValues(t, int(270), int8(14))

would return true, even though the values are not equal. Because, when
you convert int(270) to int8, it overflows and becomes 14 (270 % 256 = 14)

This commit fixes that by making sure that the conversion always happens
from the smaller type to the larger type, and then comparing the values.
Additionally, this commit also seperates out the test cases of
ObjectsAreEqualValues from TestObjectsAreEqual.

Fixes stretchr#1462
arjunmahishi added a commit to arjunmahishi/testify that referenced this issue Feb 14, 2024
The underlying function ObjectsAreEqualValues did not handle
overflow/underflow of values while converting one type to another
for comparison. For example:

    EqualValues(t, int(270), int8(14))

would return true, even though the values are not equal. Because, when
you convert int(270) to int8, it overflows and becomes 14 (270 % 256 = 14)

This commit fixes that by making sure that the conversion always happens
from the smaller type to the larger type, and then comparing the values.
Additionally, this commit also seperates out the test cases of
ObjectsAreEqualValues from TestObjectsAreEqual.

Fixes stretchr#1462
@arjunmahishi arjunmahishi self-assigned this Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug help wanted pkg-assert Change related to package testify/assert
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants