search

beehive-lab / mambo

A low-overhead dynamic binary instrumentation and modification tool for ARM (both AArch32 and AArch64 support) and RISC-V (RV64GC).
Apache License 2.0
318 stars 69 forks source link

Add assertion of ctx->thread_data to mambo_get_thread_id #113

Closed mskordal closed 7 months ago

mskordal commented 7 months ago

When in the constructor of plugins, if we call mambo_get_thread_id before mambo_register_plugin, it will cause a segmentation fault. So we added an assertion to ctx. Furthermore, if we call mambo_get_thread_id right after mambo_register_plugin while in the constructor, no thread is initialised yet which will also cause a segmentation fault due to ctx->thread_data being NULL.

IgWod commented 7 months ago

Thank you for the PR!

Two comments:

1) Please change the commit message to "Add ...". Commits should use present, not past tense. 2) I'm not convinced we need assert(ctx == NULL). I think we should safely assume that plugin is registered and context created at the point the user uses this, or in fact any, helper function. So, I'd just keep the second assert, which I'm happy with. Also, adding assert(ctx == NULL) would create a precedence for all helpers to have it, and I don't think we don't want to - the performance degradation should be marginal but still. @jkressel What do you think?

jkressel commented 7 months ago

I agree, I think we need to assume that a ctx has been created, since without it the plugin isn't registered or useful.