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 for SecBug cpp/comparison-with-wider-type #416

Merged

Conversation

kingster
Copy link
Contributor

The return type for vector.size() is size_t which is larger than uint16_t and hence an overflow can cause infinite loop.

Fixes this security bug https://codeql.github.com/codeql-query-help/cpp/cpp-comparison-with-wider-type/

@kingster kingster force-pushed the secbug-cpp/comparison-with-wider-type branch from 1d866ef to aa3419b Compare April 22, 2022 18:40
@The-EDev
Copy link
Member

The-EDev commented Apr 22, 2022

Since concurrency can be set to only a uint16_t, the maximum size for task_queue_length_pool_ would be within the limits of an unsigned 16bit integer. Please let me know if I missed anything, otherwise I will close this PR.

@kingster
Copy link
Contributor Author

In that case, maybe we should cast the size_t into uint16_t so that the security tools don't throw this as a security issue?

@The-EDev
Copy link
Member

I think I want to test the effect of adding a cast first, I don't want to hamper performance (even if very slightly) just to get a tool not to throw an incorrect error.

@kingster
Copy link
Contributor Author

True I had the same thoughts, and that's why my first proposal was to use size_t so that it doesn't effect performance at all, and the code analysis tools are also happy!

@The-EDev
Copy link
Member

Seems like the only difference between the 2 versions is a single movzx operation.. Interestingly enough this is added to the version of the code without static_cast, so at worst there's no difference and at best this version would be faster somehow..

What's your opinion @kingster?

@kingster
Copy link
Contributor Author

IMHO the size_t is a simpler solution which also avoids a cast in the loop at least (even though it doesn't affect) and is future proof even if the type of concurrency is changed in future.

@The-EDev
Copy link
Member

I would prefer it if there was a comment above the loop specifying why size_t is being used instead of uint16_t

@kingster kingster force-pushed the secbug-cpp/comparison-with-wider-type branch from 42ee395 to e2e19b5 Compare April 25, 2022 13:00
@kingster
Copy link
Contributor Author

Done. Pushed the updated changes, please have a look.

@kingster kingster force-pushed the secbug-cpp/comparison-with-wider-type branch from e2e19b5 to d49390b Compare April 25, 2022 14:46
Copy link
Member

@The-EDev The-EDev left a comment

Choose a reason for hiding this comment

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

seems alright, will merge soon.

@The-EDev The-EDev force-pushed the secbug-cpp/comparison-with-wider-type branch from d49390b to de16aab Compare April 26, 2022 02:30
@The-EDev The-EDev merged commit c953413 into CrowCpp:master Apr 26, 2022
@kingster kingster deleted the secbug-cpp/comparison-with-wider-type branch April 26, 2022 06:15
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.

2 participants