Make Launcher use ClientDescription instead of CoreId

[riesentoaster]

There may be more than one client running on each core, as per #2670.

The ClientId struct includes a reference to the CoreId, but has its own id that is unique for each client. Right now, this is independent of the LLMP ClientId, and I'm not sure I'm the right person to merge those, since I've never touched LLMP.

Also: I added overcommit to one of the example fuzzer as a (in the Makefile unused) command line arg, so users see that it exists.

@domenukk I didn't see you merged the other PR until I was already done with this…

[riesentoaster]

I also noticed that the version fixing for the formatter in the CI was still broken, so I fixed it. That's why there are so many commits…

And so many other random little improvements.

[riesentoaster]

Both remaining failing CI runs seem like they are timeouts, and they both work on my machine locally. I'm not sure why they start appearing and if we should increase the timeouts or otherwise do something to get them to behave more consistently.

[domenukk]

We already have a ClientId that's used across the codebase, see:

LibAFL/libafl_bolts/src/lib.rs

Line 175 in e32b3ea

pub struct ClientId(pub u32);

[domenukk]

I think there are two ways:
a) Name the new dude CoreDescription or similar, indicating that it's just about what core we bind to
b) Merge the two and make sure the current ClientId (a unique ID assigned by the broker(!)) gets propagated correctly to this new struct. That'd be a neat solution, but migth be more involved.

[riesentoaster]

What I would like is that the run_client function gets an additional id that uniquely identifies the client. After reading a bunch of the LLMP code, I think this is already present as the ClientId of the Sender.

1. Is this assumption correct?
2. Could I just extract this id and pass it to the function like so (last lines of Launcher::launch_with_hooks):

   let (state, mgr): (_, LlmpRestartingEventManager<_, _, _>) = builder.build().launch()?;
   let id = mgr.llmp_event_manager().llmp_client().sender().id();
   return (self.run_client.take().unwrap())(state, mgr, id, *bind_to);

   This would require the getters LlmpRestartingEventManager::llmp_event_manager and LlmpEventManager::llmp_client.
3. One significant downside with this ClientId's internals being publicly accessible is that it is not obvious where it is created/fixed. So I'm not quite sure if it is already set correctly to its final value at this point. Do you happen to know if there are any guarantees about that?

[domenukk]

Yeah that might just work, I think. But the idea about CoreId is that we can bind features like ASan to specific cores dynamically. So we would want CoreId, potentially the overcommit counter, and the unique sender ClientId.

[domenukk]

For how CoreId is used right now, see:

LibAFL/fuzzers/binary_only/qemu_launcher/src/options.rs

Line 51 in e32b3ea

#[arg(long, help = "Cpu cores to use for ASan", value_parser = Cores::from_cmdline)]

[riesentoaster]

I'll try this in another branch, too many changes to revert. And I want to leave this here in case it's ever needed again.

[riesentoaster]

@domenukk do you want to merge some form of this PR? Since there seems to be no consensus for your suggestion/PR #2684.

[riesentoaster]

Alright, i've done the renaming. Not sure why the CI breaks.

[riesentoaster]

Not sure why the CI breaks.

Ah, #2715

[riesentoaster]

Anyone know why launcher compared cores to enumerator values instead of the actual cores? I changed it, but don't know if there is a reason behind it. Just seemed odd.

Old version (example):

for (id, bind_to) in core_ids.iter().enumerate() {
    if self.cores.ids.iter().any(|&x| x == id.into()) {

[riesentoaster]

Occurrence 1

[riesentoaster]

Occurrence 2

[riesentoaster]

Occurrence 3

[riesentoaster]

@domenukk opinions? On the above and generally this PR?

Or who do I ask?

[tokatoka]

thanks for deleting this

[tokatoka]

Anyone know why launcher compared cores to enumerator values instead of the actual cores?

No idea.

but it should be the same as it is iterating over what core id cpus have. but it's better with your change

[tokatoka]

yeah looks good