Fix buffer size calculation and memory alignment in evolver_ndf15 | Avoid buffer overflow from input#652
Open
kabeleh wants to merge 3 commits intolesgourg:masterfrom
Open
Fix buffer size calculation and memory alignment in evolver_ndf15 | Avoid buffer overflow from input#652kabeleh wants to merge 3 commits intolesgourg:masterfrom
kabeleh wants to merge 3 commits intolesgourg:masterfrom
Conversation
Fix misaligned memory access in evolver_ndf15 buffer layout: The ndf15 evolver allocates a single contiguous buffer and carves it into sub-arrays for doubles, an int array (interpidx), a double-pointer array (dif), and a second double region for backward differences. The original layout placed the int array (neqp × sizeof(int)) between the double arrays and the double-pointer array: [15×neqp doubles] [neqp ints] [neqp double*] [(7×neq+1) doubles] When neqp is odd, neqp × 4 bytes leaves the subsequent double* and double regions at a 4-byte boundary, rather than the 8-byte alignment required for these types. This constitutes undefined behaviour in C11 and is flagged at runtime by the -fsanitize=undefined CCFLAG / LDFLAG as misaligned load/store errors during thermodynamics integration. The solution is to move the int array to the end of the buffer, after all double and double* regions: [15×neqp doubles] [neqp double*] [(7×neq+1) doubles] [neqp ints] Since malloc returns memory aligned to at least 8 bytes, and all regions before the int array are multiples of 8 bytes in size, every double and double* access is now aligned. The int array at the tail has no alignment requirement beyond 4 bytes, so placing it last is safe. The total buffer size is unchanged. Verified clean under -fsanitize=undefined, -fsanitize=address, and -fsanitize=thread.
There was a tiny chance that line 50 in input.h
if (flag_temp == _TRUE_) \
49
{ \
50
strcpy(destination, string_temp); \
51
}
might cause a bufferoverflow. It's very unlikely to encounter this, but no downside in preventing it.
Author
|
The second commit comes at no cost, yet prevents the (unlikely) possibility that line 50 in input.h if (flag_temp == _TRUE_) \
49
{ \
50
strcpy(destination, string_temp); \
51
}might cause a buffer overflow. It's very unlikely to encounter this, but no downside in preventing it by increasing the |
Variable length arrays are actually not supported. It's not an issue, just a compiler warning. Though, the fix is simply to define REFINE as a const int instead of just an int. Compiler is happy.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I was investigating a performance and stability issue in my own CLASS fork (kabeleh/iDM), for which I compiled CLASS with the flags
This reported a memory misalignment issue in evolver_ndf15. There is a (4 bit) int array that is in between doubles (8 bit), which causes a misalignment at the boundary. The fix is relatively simple: move the int array to the end.
This was the TL;DR. In more prose:
The ndf15 evolver allocates a single contiguous buffer and carves it into sub-arrays for doubles, an int array (interpidx), a double-pointer array (dif), and a second double region for backward differences.
The original layout placed the int array (neqp × sizeof(int)) between the double arrays and the double-pointer array:
[15×neqp doubles] [neqp ints] [neqp double*] [(7×neq+1) doubles]
When neqp is odd, neqp × 4 bytes leaves the subsequent double* and double regions at a 4-byte boundary, rather than the 8-byte alignment required for these types. This constitutes undefined behaviour in C11 and is flagged at runtime by the -fsanitize=undefined CCFLAG / LDFLAG as misaligned load/store errors during thermodynamics integration.
The solution is to move the int array to the end of the buffer, after all double and double* regions:
[15×neqp doubles] [neqp double*] [(7×neq+1) doubles] [neqp ints]
Since malloc returns memory aligned to at least 8 bytes, and all regions before the int array are multiples of 8 bytes in size, every double and double* access is now aligned. The int array at the tail has no alignment requirement beyond 4 bytes, so placing it last is safe.
The total buffer size is unchanged.
Verified clean under
-fsanitize=undefined,
-fsanitize=address, and
-fsanitize=thread.