Skip to content

Fix #847: missing range checks in statespace_*.h files#848

Merged
mhucka merged 7 commits intomasterfrom
mh-fix-847-range-checks
Aug 17, 2025
Merged

Fix #847: missing range checks in statespace_*.h files#848
mhucka merged 7 commits intomasterfrom
mh-fix-847-range-checks

Conversation

@mhucka
Copy link
Copy Markdown
Collaborator

@mhucka mhucka commented Jul 31, 2025

CodeQL scans flagged lines like 353 in lib/statespace_avx512.h, reporting that the use of offset m should follow the range check:

     while (rs[m] < csum && m < num_samples) {

In more detail:

The program contains an and-expression where the array access is defined before the range check. Consequently the array is accessed without any bounds checking. The range check does not protect the program from segmentation faults caused by attempts to read beyond the end of a buffer.

The same issue exists in the following files:

  • statespace_basic.h
  • statespace_sse.h
  • statespace_avx512.h
  • statespace_avx.h

CodeQL scans flagged lines like 353 in `lib/statespace_avx512.h`, reporting that the use of offset `m` should follow the range check:

```C++
     while (rs[m] < csum && m < num_samples) {
```

In more detail:

> The program contains an and-expression where the array access is defined before the range check. Consequently the array is accessed without any bounds checking. The range check does not protect the program from segmentation faults caused by attempts to read beyond the end of a buffer.

The same error exists in the following files:

* statespace_basic.h
* statespace_sse.h
* statespace_avx512.h
* statespace_avx.h
@github-actions github-actions Bot added the size: XS <10 lines changed label Jul 31, 2025
@mhucka mhucka self-assigned this Jul 31, 2025
@mhucka mhucka marked this pull request as ready for review August 14, 2025 02:55
@mhucka mhucka enabled auto-merge (squash) August 15, 2025 20:23
@pavoljuhas pavoljuhas disabled auto-merge August 15, 2025 21:10
Copy link
Copy Markdown
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

There seems to be one more such case:

diff --git a/lib/statespace_cuda_kernels.h b/lib/statespace_cuda_kernels.h
index b54ebca..0bc4ba7 100644
--- a/lib/statespace_cuda_kernels.h
+++ b/lib/statespace_cuda_kernels.h
@@ -318,5 +318,5 @@ __global__ void SampleKernel(unsigned num_blocks,
         FP3 im = state[l + warp_size];
         csum += re * re + im * im;
-        while (rs[m] < csum && m < num_samples) {
+        while (m < num_samples && rs[m] < csum) {
           bitstrings[m++] = k0 + k;
         }

Otherwise LGTM.

@mhucka mhucka enabled auto-merge (squash) August 15, 2025 21:46
@mhucka mhucka disabled auto-merge August 15, 2025 22:21
@mhucka
Copy link
Copy Markdown
Collaborator Author

mhucka commented Aug 17, 2025

There seems to be one more such case:

diff --git a/lib/statespace_cuda_kernels.h b/lib/statespace_cuda_kernels.h
index b54ebca..0bc4ba7 100644
--- a/lib/statespace_cuda_kernels.h
+++ b/lib/statespace_cuda_kernels.h
@@ -318,5 +318,5 @@ __global__ void SampleKernel(unsigned num_blocks,
         FP3 im = state[l + warp_size];
         csum += re * re + im * im;
-        while (rs[m] < csum && m < num_samples) {
+        while (m < num_samples && rs[m] < csum) {
           bitstrings[m++] = k0 + k;
         }

Otherwise LGTM.

Thanks for catching that.

@github-actions github-actions Bot added size: S 10< lines changed <50 and removed size: XS <10 lines changed labels Aug 17, 2025
@mhucka
Copy link
Copy Markdown
Collaborator Author

mhucka commented Aug 17, 2025

The latest commit has that fixed.

@mhucka mhucka enabled auto-merge (squash) August 17, 2025 02:48
@mhucka mhucka merged commit f69ef7d into master Aug 17, 2025
49 checks passed
@mhucka mhucka deleted the mh-fix-847-range-checks branch August 17, 2025 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: S 10< lines changed <50

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants