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

Proposed fix for #3094, issue with trace-cmp #3095

Merged
merged 1 commit into from
Mar 21, 2025

Conversation

DanBlackwell
Copy link
Contributor

Description

There is an issue with one of the macros for generating __libafl_targets_value_profileX calls, which resulted in all instances calling __libafl_targets_value_profile1 - meaning that only 1 byte of the conditional check got passed through.

@ammaraskar: you said that you found the bug due to some unexpected results (not being able to break through magic numbers); any chance you can rerun your setup and see if this fixes it?

Also: not sure if the formatter setup has changed or what - let me know if you'd prefer I remove the whitespace changes from this PR and create a new one after. Only libafl_targets/src/sancov_cmp.c:17 has changed semantically.

Checklist

  • I have run ./scripts/precommit.sh and addressed all comments

…arisons are treat as 1 byte (rather than 2, 4, or 8)
@domenukk
Copy link
Member

One of the CI fuzzers doesn't find any objectives now(?)
I'll restart it and see if it was just flakey

@DanBlackwell
Copy link
Contributor Author

Hmm, looks like it passed this time but got Error: Process completed with exit code 1. the first time. I would understand an OOM maybe (137 IIRC), but that sounds like the fuzzer terminated itself due to bad state or something?

@domenukk
Copy link
Member

@tokatoka can you take a look plz :)

@domenukk domenukk requested a review from tokatoka March 20, 2025 11:15
@tokatoka
Copy link
Member

Hmm, looks like it passed this time but got Error: Process completed with exit code 1. the first time. I would understand an OOM maybe (137 IIRC), but that sounds like the fuzzer terminated itself due to bad state or something?

which fuzzer is this about?

@tokatoka
Copy link
Member

yeah it looks good

@addisoncrump
Copy link
Collaborator

This deserves a fuzzbench run. Should I fire this off locally?

@tokatoka
Copy link
Member

yeah

@addisoncrump
Copy link
Collaborator

Okay, I'll just run like 3 trials for 6 hours, this difference should be plain by this alone.

@tokatoka
Copy link
Member

but you have to change fuzzbench/src/lib.rs
it is not using value profile now

@ammaraskar
Copy link

ammaraskar commented Mar 20, 2025

Here's a minimal recreator from the test case I was using:

struct Data {
    uint16_t first;
    uint32_t second;
} __attribute__((packed));

extern "C" int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
    if (Size < sizeof(struct Data)) {
        return 0;
    }
    struct Data* data = (struct Data*) Data;

    // XOR so value profiles are needed, comparison logging not enough.
    if ((data->first ^ 0xdead) == 0xbeef &&
        (data->second ^ 0xdeadbeef) == 0xaabbccdd) {
        abort();
    }

    return 0;
}

I can confirm that with this change it is found successfully by libafl-libfuzzer whereas before it couldn't!

$ ./libafl-libfuzzer -use_value_profile=1
[Testcase    #0]  (GLOBAL) run time: 0h-0m-1s, clients: 1, corpus: 22, objectives: 0, executions: 0, exec/sec: 0.000, cmps: 0.037%, edges: 80.000%, size_edges: 80.000%, stability: 100.000%
                  (CLIENT) corpus: 22, objectives: 0, executions: 0, exec/sec: 0.000, cmps: 3/8192 (0%), edges: 4/5 (80%), size_edges: 4/5 (80%), stability: 3/3 (100%)
[2025-03-20T15:20:32Z ERROR libafl::executors::hooks::unix::unix_signal_handler] Crashed with SIGABRT
[2025-03-20T15:20:32Z ERROR libafl::executors::hooks::unix::unix_signal_handler] Child crashed!
[2025-03-20T15:20:32Z ERROR libafl::executors::hooks::unix::unix_signal_handler] input: "ee0ec739bda2f018"

Copy link

@ammaraskar ammaraskar left a comment

Choose a reason for hiding this comment

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

Semantic change looks good and passes test. No opinion on the unrelated formatting stuff, maybe a LibAFL maintainer can chime in on that.

@tokatoka
Copy link
Member

format is fine. as long as ci doesn't complain i don't care

@tokatoka
Copy link
Member

@addisoncrump you've run it?

@tokatoka tokatoka merged commit 9195245 into AFLplusplus:main Mar 21, 2025
109 checks passed
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.

5 participants