Skip to content

Fix buffer overflow in CPU block_ldlt in find_maxloc on NaNs #234

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

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

Fix buffer overflow in CPU block_ldlt in find_maxloc on NaNs #234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
114a7fd
Fix buffer overflow in CPU block_ldlt in find_maxloc on NaNs
kkofler Apr 1, 2025
6299940
Fix crash in CPU BuddyAllocator ~Page when block_ldlt throws
kkofler Apr 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/ssids/cpu/BuddyAllocator.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,6 @@ public:
#endif /* MEM_STATS */
}
~Page() noexcept(false) {
if(next_ && head_[nlevel-1] != 0)
throw std::runtime_error("outstanding allocations on cleanup\n");

@mjacobse mjacobse Apr 2, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I do not understand why this can just be removed and how it relates to the other changes.

@kkofler kkofler Apr 2, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Is it safe to just remove? To be honest, I do not know. I just know that it did not crash when I removed this check, so I can only hope it did not corrupt memory somewhere.

What I do know is that if I do not remove this check, then the exception I throw with the if (t >= BLOCK_SIZE || m >= BLOCK_SIZE) throw SingularError(p); in block_ldlt.hxx causes ~Page to be run in this (next_ && head_[nlevel-1] != 0) state, so it throws that std::runtime_error, and std::terminate gets called as a result. So something has to be done about that.

If removing the check is not safe, then we need another way to handle this.

@kkofler kkofler Apr 2, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It might also be that existing exceptions thrown in block_ldlt.hxx by this check:

      if(fabs(bestv) < small) {
         if(!action) throw SingularError(p);

can also trigger this error case, but I am not sure about that either. It might also be that it only gets triggered because I am throwing an exception also in the case of a non-zero action (i.e., that that is the context in which the exception is unexpected). I do not understand the allocator usage well enough yet to know the answer.

if(next_) {
typename IntAllocTraits::allocator_type intAlloc(alloc_);
IntAllocTraits::deallocate(intAlloc, next_, 1<<(nlevel-1));
Expand Down
7 changes: 6 additions & 1 deletion src/ssids/cpu/kernels/block_ldlt.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,9 @@ void find_maxloc(const int from, const T *a, int lda, T &bestv_out, int &rloc, i
cloc = bc2[i].i;
}
}
bestv_out = a[cloc*lda+rloc];
bestv_out =
(cloc < BLOCK_SIZE && rloc < BLOCK_SIZE) ? a[cloc*lda+rloc]
: 0.0;
}

/** Returns true if a 2x2 pivot can be stably inverted.
Expand Down Expand Up @@ -298,6 +300,9 @@ void block_ldlt(int from, int *perm, T *a, int lda, T *d, T *ldwork,
int t, m; // row and col location of maximum entry
find_maxloc<T,BLOCK_SIZE>(p, a, lda, bestv, t, m);

// Handle case where find_maxloc failed (e.g., due to NaNs in the input)
if (t >= BLOCK_SIZE || m >= BLOCK_SIZE) throw SingularError(p);

@mjacobse mjacobse Apr 2, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not quite sure about this check. It unconditionally gives up with SingularError, ignoring the action setting. Also, if find_maxloc failed, the change above will cause bestv to be 0.0, and so fabs(bestv) < small will be true, so we should go into the handling of the singular case anyways, right?

@kkofler kkofler Apr 2, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Well, if find_maxloc fails, no matter in what stage, it is always an error.

If I do not add this check, i.e., if I add only the bestv_out buffer overflow fix without this added check, then it continues attempting to compute with invalid indices (INT_MAX is not a valid index to work with) and eventually (I think in a later iteration, because in this one, we should have t==INT_MAX and m==INT_MAX and hence t==m, and so pivsiz should always be 1) runs into the

      if(pivsiz == 0) {
         // FIXME: debug remove
         printf("broken!\n");
         …
         exit(1);
      }

crash case. Assuming it gets so far to begin with, because a11 = a[t*lda+t]; is also an invalid memory access if t is INT_MAX.

@kkofler kkofler Apr 2, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

But in my tests, the "broken!" error is what I hit without this added check.


// Handle case where everything remaining is small
// NB: There might be delayed columns!
if(fabs(bestv) < small) {
Expand Down