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

fix snmalloc::memmove reverse block copy. #689

Closed

Conversation

devnexen
Copy link
Collaborator

No description provided.

@SchrodingerZhu
Copy link
Collaborator

The fuzzer is still unhappy:

schrodinger@Homotopy:~/snmalloc/build$ ./fuzzing/snmalloc-fuzzer --fuzz=snmalloc_fuzzing.forward_memmove
[.] Sanitizer coverage enabled. Counter map size: 47091, Cmp map size: 262144
Note: Google Test filter = snmalloc_fuzzing.forward_memmove
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from snmalloc_fuzzing
[ RUN      ] snmalloc_fuzzing.forward_memmove
FUZZTEST_PRNG_SEED=fQDApsy7McK2J43PQRmqCPzNKE6Ys0BH1oQ8mhHa2ZA
[*] Corpus size:     1 | Edges covered:     15 | Fuzzing time:       1.890133ms | Total runs:  1.00e+00 | Runs/secs:   529 | Max stack usage:      304
[*] Corpus size:     2 | Edges covered:     16 | Fuzzing time:       1.979135ms | Total runs:  2.00e+00 | Runs/secs:  1010 | Max stack usage:      304
[*] Corpus size:     3 | Edges covered:     17 | Fuzzing time:       2.044536ms | Total runs:  7.00e+00 | Runs/secs:  3423 | Max stack usage:      304
[*] Corpus size:     4 | Edges covered:     18 | Fuzzing time:       2.389542ms | Total runs:  3.80e+01 | Runs/secs: 15902 | Max stack usage:      304
[*] Corpus size:     5 | Edges covered:     18 | Fuzzing time:       2.552945ms | Total runs:  4.90e+01 | Runs/secs: 19193 | Max stack usage:      304
[*] Corpus size:     6 | Edges covered:     18 | Fuzzing time:       2.882951ms | Total runs:  5.40e+01 | Runs/secs: 18730 | Max stack usage:      304
[*] Corpus size:     7 | Edges covered:     21 | Fuzzing time:       3.232157ms | Total runs:  6.80e+01 | Runs/secs: 21038 | Max stack usage:      304
[*] Corpus size:     8 | Edges covered:     22 | Fuzzing time:        3.41106ms | Total runs:  7.30e+01 | Runs/secs: 21400 | Max stack usage:      304
[*] Corpus size:     9 | Edges covered:     22 | Fuzzing time:       3.647064ms | Total runs:  8.80e+01 | Runs/secs: 24128 | Max stack usage:      304

=================================================================
=== Fuzzing stats

Elapsed time: 3.789866ms
Total runs: 91
Edges covered: 22
Total edges: 47091
Corpus size: 9
Max stack used: 304

=================================================================
=== BUG FOUND!

/home/schrodinger/snmalloc/fuzzing/snmalloc-fuzzer.cpp:190: Counterexample found for snmalloc_fuzzing.forward_memmove.
The test fails with input:
argument 0: "VV\341VssV\025"
argument 1: 2

=================================================================
=== Regression test draft

TEST(snmalloc_fuzzing, forward_memmoveRegression) {
  forward_memmove(
    "VV\341VssV\025",
    2
  );
}

Please note that the code generated above is best effort and is intended
to use be used as a draft regression test.
For reproducing findings please rely on file based reproduction.
[!] Failed to write reproducer file!

=================================================================
Aborted

The fuzzer seems to run correctly when using std::memmove.

@devnexen
Copy link
Collaborator Author

devnexen commented Nov 17, 2024

was just to fix the asan crash, I ll look at the rest later. Seems Arch::copy has a bug too for both memcpy/memmove tough.

@SchrodingerZhu
Copy link
Collaborator

SchrodingerZhu commented Nov 17, 2024

I think the following fixes the issue:

copy_one<Size>(
        pointer_offset(dst, (size_t)i - Size),
        pointer_offset(src, (size_t)i - Size));
    }

and

    ptrdiff_t diff = static_cast<ptrdiff_t>(address_cast(dst)) -
      static_cast<ptrdiff_t>(address_cast(src));
    ptrdiff_t signed_length = static_cast<ptrdiff_t>(len);
    if (diff > 0 && diff < signed_length)
    {
      // slow 'safe' reverse copy, we avoid optimised rollouts
      // to cope with typical memmove use cases, moving
      // one element to another address within the same
      // contiguous space.
      block_reverse_copy<1>(dst, src, len);
      return dst;
    }
    if (diff > -signed_length && diff < 0)
    {
      // slow 'safe' forward copy, we avoid optimised rollouts
      // to cope with typical memmove use cases, moving
      // one element to another address within the same
      // contiguous space.
      block_copy<1>(dst, src, len);
      return dst;
    }

@devnexen
Copy link
Collaborator Author

Seems these your last changes all your fuzzer tests pass with.

@devnexen devnexen force-pushed the snmalloc_fix_memmove_reverse_loop branch from 8730d12 to 36a3f2e Compare November 17, 2024 07:16
@nwf
Copy link
Collaborator

nwf commented Nov 17, 2024

I haven't been paying attention, but it looks like the snmalloc memmove implementation's "slow safe" case(s), before and after this PR, are not correct for CHERI, since they move byte at a time regardless of alignment?

@mjp41
Copy link
Member

mjp41 commented Nov 19, 2024

@devnexen, to address Wes's comment about CHERI, I think we really need to have a per Arch implementation of move. Then on CHERI this can detect cases where pointers might exist and ensure that the tags are preserved.

I think this is a larger change than you might want to do. I propose we revert #593 for now, and then we can add back support later. This way there is no time pressure to get this fixed. Is that okay with you?

@devnexen
Copy link
Collaborator Author

Yes that s the right thing to do. As for implementing CHERI I m not sure I really can w/o related h/w.

@devnexen devnexen closed this Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants