Skip to content

C++: Account for the signedness of the lesser operand in cpp/comparison-with-wider-type #13502

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

Merged

Conversation

rvermeulen
Copy link
Contributor

The provided example case is not detected because the value INT16_MAX + 1 can be represented with 16 bits. To account for the overflow that happens before the comparison we need to determine if the value can be represented by the type of the lesser operand. We do this by adjusting the size of the lesser operand if it is signed to ensure that the check
upperBound(conv).log2() > (getComparisonSize(small) * 8 - getComparisonSizeAdjustment(small)) holds.

...
	uint32_t big_num = INT32_MAX;
	char buf[big_num];
	int16_t bytes_received = 0;
	int max_get = INT16_MAX + 1;

	// BAD: 'bytes_received' is compared with a value of a wider type.
	// 'bytes_received' overflows before  reaching 'max_get',
	// causing an infinite loop
	while (bytes_received < max_get)
		bytes_received += get_from_input(buf, bytes_received);
	}
...

@rvermeulen rvermeulen requested a review from a team as a code owner June 20, 2023 00:03
@github-actions github-actions bot added the C++ label Jun 20, 2023
@MathiasVP
Copy link
Contributor

The code (and the DCA run) LGTM. Final thing: Do we want to add a change note for this? Something like:

---
category: minorAnalysis
---
* The `cpp/comparison-with-wider-type` query now correctly handles relational operations on signed operators. As a result the query may find more results.

This would need to go to a new file at cpp/ql/src/change-notes.

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM!

@MathiasVP MathiasVP merged commit 95ddc01 into github:main Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants