-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix unchecked allocations #2844
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
base: next
Are you sure you want to change the base?
Conversation
Mapping.c
Outdated
|
|
||
| if (*cache == NULL) | ||
| *cache = make_id2insn(insns, max); | ||
| if (!(*cache = make_id2insn(insns, max))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are only two places where handle->insn_id is actually called. Wouldn't it be way simple to just initialize the cache in cs_disasm and cs_disasm_iter?
This would save us all the other changes in the other functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can move the checked allocation into a new routine which is common between cs_disasm and cs_disasm_iter...
That way the arch-specific ID-translation code can just assume that any needed cache has already been allocated.
But this does mean that we'll need to provide a way for the architecture-init code to signal how much cache it's associated translation table needs.
That will basically entail a new handle->required_insn_cache_size field (which will be zero for archetectures that don't need it).
Does that work for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this makes sense.
To give some context. The reason I'd like to have as few allocations in the modules as possible is for removing allocations completely in the (far) future.
This makes it potentially easier to refactor then, if we want to introduce a stack only disassembly logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, bc5f6b2 should address this.
Lemmie know if that's sufficient.
6fa69eb to
bc5f6b2
Compare
|
Looked briefly and seems good. But can you please fix the build? |
bc5f6b2 to
2a419b6
Compare
|
A missing semicolon? |
|
No problem
Makefile is deprecated though (we still run and fix the build, but don't extend it for developer tasks). I'd recommend to switch to cmake. |
|
@Grond66 There are memory issues. Please rebase though before. I just merged a big RISC-V update. |
Your checklist for this pull request
Detailed description
Capstone's source has a few places where memory is allocated, but there are no checks for out-of-memory (OOM) conditions. This can cause problems in memory-constrained environments; instead of having an operation error out with
CS_ERR_MEM, we get a null-pointer dereference. This is fairly simple to fix, but it does touch quite a few files. Please let me know if you want any improvements or further explanation of the fixes.Test plan
The standard test suite should do fine.
Closing issues
None, but I can open a tracking issue if that's helpful.