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.

Sign up for GitHub

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

Jump to bottom

Fix CodeQL reports on crunch and crnlib #60

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix CodeQL reports on crunch and crnlib #60

wants to merge 4 commits into from

Conversation

illwieckz
Copy link
Member

@illwieckz illwieckz commented Jul 6, 2024 •
edited
Loading

Fix CodeQL reports.

The following cpp/static-buffer-overflow critical error is dismissed as it is a false positive:

Potential buffer-overflow: 'm_buf' has size 2 but 'm_buf[3]' may be accessed here.

It is a false positive because the tool fails to understand that the value tested for the switch case is the array size itself.

@illwieckz
Copy link
Member Author

That doesn't work… :-/

@illwieckz illwieckz marked this pull request as draft July 6, 2024 03:08
@illwieckz illwieckz changed the title Attempt to silence false positive CodeQL cpp/static-buffer-overflow WIP: Attempt to silence false positive CodeQL cpp/static-buffer-overflow Jul 6, 2024
@slipher
Copy link
Member

slipher commented Jul 6, 2024

This analyzer has a diff-only mode, right? (The one where it comments on pull requests.) So there is really no need to restructure everything to please the computer.

@illwieckz
Copy link
Member Author

Yes, and one can dismiss some reports, but I would have preferred if the tool was happy without doing manual tricks in the tool UI as moving or copying the code or just restyling it would bring the false positive back. I was hoping a simple rewrite may help it to get it, it looks like not.

@illwieckz
Copy link
Member Author

It is also very likely that GitHub would report the false positive on every fork even if we dismiss it there.

@illwieckz illwieckz force-pushed the illwieckz/codeql-fixes branch from f7bbf3e to fdf1b61 Compare July 10, 2024 21:28
@illwieckz
Copy link
Member Author

Well, there is no way to avoid that false positive report, I don't know how to report it to CodeQL but they definitely fail to take in account this is a template and they should not test array[N] with an index greater than N. In fact I actually expect the compiler to remove any alternative code, and then remove the code I may write to silence the CodeQL report.

@illwieckz illwieckz changed the title WIP: Attempt to silence false positive CodeQL cpp/static-buffer-overflow WIP: Attempt to fix CodeQL reports Jul 10, 2024
@illwieckz illwieckz force-pushed the illwieckz/codeql-fixes branch 2 times, most recently from 951d912 to ed5344a Compare July 10, 2024 22:03
@illwieckz illwieckz force-pushed the illwieckz/codeql-fixes branch from e3ce3ff to c95c447 Compare July 10, 2024 22:26
@illwieckz
Copy link
Member Author

The commit fixing stb_image.h was also reported to:

@illwieckz
Copy link
Member Author

illwieckz commented Jul 10, 2024 •
edited
Loading

Interesting, something broke the tool!

@illwieckz illwieckz force-pushed the illwieckz/codeql-fixes branch 2 times, most recently from 52ac792 to 5abcc45 Compare July 10, 2024 22:55
@illwieckz
Copy link
Member Author

Interesting, something broke the tool!

Fixed.

@illwieckz
Copy link
Member Author

So there is nothing left reported by CodeQL, the false-positive template ones were dismissed, all the other ones were fixed.

@illwieckz
Copy link
Member Author

illwieckz commented Jul 10, 2024 •
edited
Loading

The choice of doing static_cast<double>(x) or (double)x was decided by what was doing the code living immediately before or after the change, to keep the wording consistent. When the wording is not consistent within a file, it means the file was already not consistant among its various parts, and I made sure the wording was consistent within each parts.

@illwieckz illwieckz changed the title WIP: Attempt to fix CodeQL reports Fix CodeQL reports Jul 10, 2024
@illwieckz illwieckz marked this pull request as ready for review July 10, 2024 23:16
@illwieckz illwieckz changed the title Fix CodeQL reports Fix CodeQL reports on crunch and crnlib Jul 10, 2024
@illwieckz illwieckz force-pushed the illwieckz/codeql-fixes branch 2 times, most recently from c4357a9 to 85c1749 Compare July 12, 2024 02:44
slipher
slipher reviewed Jul 14, 2024
View reviewed changes
crunch/crunch.cpp
@@ -958,7 +958,7 @@ class crunch {
else {
console::info("CRN texture info:");

console::info("Width: %u, Height: %u, Levels: %u, Faces: %u\nBytes per block: %u, User0: 0x%08X, User1: 0x%08X, CRN Format: %u",
console::info("Width: %u, Height: %u, Levels: %u, Faces: %u\nBytes per block: %u, User0: 0x%08X, User1: 0x%08X, CRN Format: %l",
Copy link
Member

Choose a reason for hiding this comment

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

This one doesn't quite fix the issue... apparently the crn_format enum, having both a -1 value and a 2^32 - 1 value, is a different size depending on the compiler. In MSVC it is 32 bits but in some others it must be 64. We could force it to be a specific type with the C++11 feature enum crn_format: uint32_t or whatever, instead of the old approach of trying to influence the type by adding fake values.

crnlib/crn_image_utils.cpp
@@ -570,9 +570,9 @@ bool resample_multithreaded(const image_u8& src, image_u8& dst, const resample_p
return false;

p.m_pSrc_pixels = src_samples.get_ptr();
p.m_src_pitch = src_width * resampler_comps * sizeof(float);
p.m_src_pitch = (size_t)src_width * (size_t)resampler_comps * sizeof(float);
Copy link
Member

Choose a reason for hiding this comment

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

Crunch has a maximum image dimension of 4096 so we don't ever need to worry about super-sized memory allocations.

crnlib/crn_clusterizer.h
@@ -53,7 +53,7 @@ class clusterizer {
root.m_total_weight += weight;
root.m_vectors.push_back(i);

ttsum += v.dot(v) * weight;
ttsum += (double)v.dot(v) * (double)weight;
Copy link
Member

Choose a reason for hiding this comment

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

Can we just disable float/double precision warnings please? It simply doesn't matter. This is a lossy image compression program.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is CodeQL static analysis report, not a compiler warning.

Copy link
Member

Choose a reason for hiding this comment

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

I know that. It should be configurable.

@illwieckz illwieckz force-pushed the illwieckz/codeql-fixes branch from 85c1749 to b5f707f Compare July 15, 2024 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slipher slipher slipher left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@illwieckz @slipher