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

Please highlight that stack arrays are unprotected #4

Open
hal-ler opened this issue Apr 24, 2017 · 2 comments
Open

Please highlight that stack arrays are unprotected #4

hal-ler opened this issue Apr 24, 2017 · 2 comments
Assignees

Comments

@hal-ler
Copy link

hal-ler commented Apr 24, 2017

As title says, people should not have to read through every line of the code to know about such limitations. Especially since the paper talks so much about the importance of handling arrays well, yet never mentions this limitation.

@dimakuv dimakuv self-assigned this Apr 24, 2017
@dimakuv
Copy link
Member

dimakuv commented Apr 24, 2017

Just to clarify, the lines you refer to are:

void visitAlloca(AllocaInst* AI) {
  if (AI->isArrayAllocation()) {
    ... special non-protected case ...

This case is for alloca() and variable-length arrays (VLAs) (in C99). In LLVM terms, this is e.g. alloca i32, i32 size. Calling it stack arrays is misleading; most arrays allocated on stack have constant size, which in LLVM looks like alloca [10 x i32] (array of size ten).

These special cases were triggered only in a couple uninteresting functions, so I didn't feel a pressing need to implement a working solution. But agreed, I should better implement it.

Note for myself: insert additional alloca i32 and make its address as the upper bound of a VLA. This requires knowledge in which direction the stack grows. (How does AddressSanitizer deal with it?!)

@hal-ler
Copy link
Author

hal-ler commented Apr 24, 2017

Technically speaking, from the IR perspective both representations are fine. I forgot which one the front-end actually uses and it seems that you are right that the ArrayType-based option is the canonical form. Still it is best not to rely on these kind of things, since passes are not required to follow suite. One option is to make sure that the set of InstCombine passes is always executed before this pass (specifically the simplifyAllocaArraySize function in InstCombineLoadStoreAlloca.cpp).

I would also not rely on alloca ordering since too much can happen (including somebody combining the pass with SafeStack without understanding the implications). Why don't you just extend the array with the right size? You already create a brand new alloca for the array. Also be careful with code around allocas, since you can easily end up transforming static allocas into dynamic ones (by having them preceded by instrumentation from a previous alloca).

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

No branches or pull requests

2 participants