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

v4.3.1 broke 8-byte alignment of some structs #1928

Closed
vthib opened this issue Jun 17, 2023 · 1 comment
Closed

v4.3.1 broke 8-byte alignment of some structs #1928

vthib opened this issue Jun 17, 2023 · 1 comment
Labels

Comments

@vthib
Copy link
Contributor

vthib commented Jun 17, 2023

Describe the bug

On v4.3.1, some YR_MATCH can be 4-byte aligned instead of 8-byte aligned, leading to non-aligned reads at least on Windows MSVC x86.
This leads to panics in yara-rust as all structs are expected to be properly aligned.

This is a regression caused by 143edac.

The issue is that all notebook allocations start with this code:

// Round up the size to a multiple of 8, which also implies that the returned
// pointers are aligned to 8 bytes. The 8-byte alignment is required by some
// platforms (e.g. ARM and Sparc) that have strict alignment requirements when
// deferrencing pointers to types larger than a byte.
size = (size + 7) & ~0x7;

But this is only true as long as the initial page pointer of the page is already 8-byte aligned.
This used to be true before 143edac, as the definition of the page was:

struct YR_NOTEBOOK_PAGE
{
  // Amount of bytes in the page that are actually used.
  size_t used;
  // Pointer to next page.
  YR_NOTEBOOK_PAGE* next;
  // Page's data.
  uint8_t data[0];
};

Since the page is allocated with malloc which guarantees a 8-byte alignment, the data pointer was also 8-byte aligned (at least on 32-bit and 64-bit systems, not on smaller ones).

But the aforementioned commit brought a new field in the struct:

struct YR_NOTEBOOK_PAGE
{
  // Size of this page.
  size_t size;
  // Amount of bytes in the page that are actually used.
  size_t used;
  // Pointer to next page.
  YR_NOTEBOOK_PAGE* next;
  // Page's data.
  uint8_t data[0];
};

This does not break anything on 64-bit systems, since all fields are 8-byte long. But this breaks on 32-bits system, where the data pointer is now 4-byte aligned.

To Reproduce

For example, adding this assert in scan.c:

new_match = yr_notebook_alloc(
  context->matches_notebook, sizeof(YR_MATCH));
assert(((intptr_t) new_match) & 0x7 == 0);

Then running the tests on a 32-bit system will lead to the assert failing.

Expected behavior

As documented in the notebook, all allocations should be 8-byte aligned.

@plusvic
Copy link
Member

plusvic commented Jun 30, 2023

Should be fixed in #1930

@plusvic plusvic closed this as completed Jun 30, 2023
poliorcetics pushed a commit to JustRustThings/yara-rust that referenced this issue Sep 12, 2023
poliorcetics pushed a commit to JustRustThings/yara-rust that referenced this issue Nov 16, 2023
poliorcetics pushed a commit to JustRustThings/yara-rust that referenced this issue Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants