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 regressions in custom memory allocator support #2251

Merged
merged 1 commit into from
Jan 20, 2024

Conversation

oleavr
Copy link
Contributor

@oleavr oleavr commented Jan 18, 2024

Where new code started using malloc()/calloc()/free() directly instead of going through cs_mem_*().

Where new code started using malloc()/calloc()/free() directly instead
of going through cs_mem_*().
MCInst_insert0(MI, VCCPos + 3, Op);
free(Op);
MCOperand Op = *MCInst_getOperand(MI, TiedOp);
MCInst_insert0(MI, VCCPos + 3, &Op);
Copy link
Collaborator

@Rot127 Rot127 Jan 19, 2024

Choose a reason for hiding this comment

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

Not sure about this change. The free() was definitely wrong. But MCInst_insert0() doesn't move Op, but only saves the pointer. So it should be copied on the heap.

Could you take a look at it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes a copy in the assignment it does here while dereferencing the pointer:

inst->Operands[index] = *Op;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah of cause. Missed the line.

Copy link
Collaborator

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

@kabeor
Copy link
Member

kabeor commented Jan 20, 2024

LGTM, thanks!

@kabeor kabeor merged commit 31ea133 into capstone-engine:next Jan 20, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants