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

regression on ALIGNED_MEMORY with manual symbolization #889

Closed
PixelRick opened this issue Apr 25, 2020 · 7 comments
Closed

regression on ALIGNED_MEMORY with manual symbolization #889

PixelRick opened this issue Apr 25, 2020 · 7 comments

Comments

@PixelRick
Copy link
Contributor

Aligned memory optimization is no longer working in API::createSymbolicMemoryExpression and API::symbolizeMemory since e6122cc.
In these methods the symbolic expressions created specifically for the aligned memory map only rely on the map itself to stay alive, but they no longer can:

if (this->modes->isModeEnabled(triton::modes::ALIGNED_MEMORY)) {
  const SharedSymbolicExpression& aligned = this->newSymbolicExpression(node, MEMORY_EXPRESSION, "Aligned Byte reference - " + comment);
  this->addAlignedMemory(address, writeSize, aligned);
}
@JonathanSalwan
Copy link
Owner

Erf.... Thanks for this report @PixelRick. I will try to fix this issue asap (probably monday)

@JonathanSalwan
Copy link
Owner

JonathanSalwan commented Apr 25, 2020

I cannot wait until Monday...

@JonathanSalwan
Copy link
Owner

JonathanSalwan commented Apr 25, 2020

Can you tel me if it works on your side? @fvrmatteo can you also test on your programs?

@PixelRick
Copy link
Contributor Author

PixelRick commented Apr 26, 2020

I'll try it out tomorrow, got notified a bit late.

Looking into it, I realized that in assignSymbolicExpressionToMemory the split in byte refs happens on the given expression's node instead of a reference node to the given expression. Is it intended this way ?

If the answer is "not necessarily", what do you think about having the split happen on a reference to the SE of the full memory access (created or given) in the 3 related methods ?
This would make the byte refs share the ownership of the aligned SE.. which reduces the lifetime of the latter to what I suspect you intended initially.
It also makes the aligned memory mode to not change the asts whether its enabled or not, but only point to the hidden main SE that would otherwise be hidden behind a concat.

As for which SEs are referenced in the instructions, if we have the aligned SE instead of the per byte ones then the nodes retrieved through getMemoryAst are ensured to always contain a reference to it. Necessary for backward slicing if I'm not missing something there..

edit: I cherry picked the commit onto my branch and it's working again.

@JonathanSalwan
Copy link
Owner

edit: I cherry picked the commit onto my branch and it's working again.

Thanks, I think I will release a v0.8.1 to have a release which is working properly, and then, we can discuss about how to optimize these SE construction in the v0.9

@JonathanSalwan
Copy link
Owner

I'm closing this as it's fixed but I keep in mind that could do a potential refactoring for the dev0.9. Thanks @PixelRick for this bug report. I've pushed a new release fixing this issue.

@JonathanSalwan JonathanSalwan added this to the v0.8 milestone Apr 28, 2020
@fvrmatteo
Copy link
Contributor

Sorry for the delay @JonathanSalwan, I've been super busy, but I finally managed to test the fix using Triton on my tools and I can confirm that the issue I had with that same commit is now fixed with this v0.8.1 release! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants