Page MenuHomePhabricator

[PoisonChecking] Validate inbounds annotation on getelementptr where possible
Needs ReviewPublic

Authored by reames on Jul 9 2019, 2:49 PM.

Details

Summary

The rules for inbounds are a bit subtle, so I'd appreciate another set of eyes. Please review the usage of getUnderlyingObject and getObjectSize carefully; my intent is to factor this out for use in inferring inbounds from the optimizer in the near future, so having it be correct is important.

Diff Detail

Event Timeline

reames created this revision.Jul 9 2019, 2:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2019, 2:49 PM

Three comments, though I need to look at it when I'm actually awake.

lib/Transforms/Instrumentation/PoisonChecking.cpp
208

GlobalVariable is another easy one, and isMallocLikeFn.

213

You use GetUnderlyingObject to get a base which is used in checks later but you look at the indices of this GEP only.

I think that is not OK, e.g., if you have two geps, one all positive indices and the one before negative ones.

234

The in bounds addresses for an allocated object are all the addresses that point into the object, plus the address one byte past the end.

You might need to adjust SizeInBytes by one, though it's late.

fhahn added a subscriber: fhahn.Jul 10 2019, 9:23 AM
nlopes added inline comments.Jul 10 2019, 9:33 AM
lib/Transforms/Instrumentation/PoisonChecking.cpp
237

Doing these pointer comparisons isn't legal, and LLVM can fold "out_of_bonds_ptr >= valid_ptr" to poison.
You need to cast those pointers to integers and do the comparison there: offset >= 0 && offset <= obj_size. You may want to lower the GEP in IR right away (there's a function for that in ValueTracking or somewhere else, I forget)?

reames planned changes to this revision.Jul 10 2019, 10:04 AM
reames marked 4 inline comments as done.
reames added inline comments.
lib/Transforms/Instrumentation/PoisonChecking.cpp
208

GlobalVariable was a bit unclear to me since I haven't studied the rules for conflicting definitions in different modules.

isMallocLikeFn is definitely valid, but requires TLI (which this pass currently doesn't have). I was planning on doing that in a follow up.

213

Good catch! Yeah, definitely needs fixed.

234

This should be handled by using UGT not UGE right?

237

Can you cite where in the LangRef it says this? I think you're quite possibly right, but a) I didn't find the wording with a quick search and b) the output seems to survive the optimizer in simple cases which is maybe a hint we don't implement it that way?

reames updated this revision to Diff 209008.Jul 10 2019, 10:16 AM

Fix bug caught in review. Thanks!

jdoerfert added inline comments.Jul 10 2019, 10:32 AM
lib/Transforms/Instrumentation/PoisonChecking.cpp
208

That is fine with me.

217

While I now thing this is correct, we might want to make it less restrictive using existing functionality to accumulate and strip pointer stuff, e.g., along the lines of:

const Value *Base = GEP->stripAndAccumulateConstantOffsets(const DataLayout &DL,
                                                 APInt &Offset,
                                                 bool AllowNonInbounds);
IsNonNegativeOffset = (Base == Obj) && Offset >= 0;
234

Yes. An offset of one in the right direction, compared to what you have now, is required. Either change is fine.

237

I would have thought the reasoning described by @nlopes is only valid for inbounds GEPs. "Normal" GEPs should be allowed to go out-of-bounds without producing poison (IMHO).

aqjune added inline comments.Jul 14 2019, 1:34 PM
lib/Transforms/Instrumentation/PoisonChecking.cpp
234

Is this case covered as well?

a = alloca i32 // 4 bytes
p1 = gep a, -1
p2 = gep inbounds p1, 2

p2 is poison because p1 is not in-bounds pointer.
UpperCheck will succeed because Addr is smaller than a + 4.

300

A silly question: where is the poison-ness of operands of GEP checked at?

jdoerfert added inline comments.Jul 14 2019, 4:56 PM
lib/Transforms/Instrumentation/PoisonChecking.cpp
300

I think this is relates to your other question: nowhere yet (I think)

Though, this is a really good point. The check for an inbound gep could include one to ensure a valid base pointer. How to do that is tricky though. If the base allocation is known, one can substract the base pointer from the base allocation and do diff >= 0 && diff <= length. If the base allocation is not known, we would need to track more information throughout the program. There are different ways to do this but I guess the first case should be covered next.

reames planned changes to this revision.Jul 30 2019, 11:25 AM

Just indicating in the review state that the next action item here is mine. Probably not going to get back to this for a bit, so want that to be clear to reviewers.

reames marked 3 inline comments as done.Wed, Aug 21, 1:20 PM

Address comments in advance of patch refresh.

lib/Transforms/Instrumentation/PoisonChecking.cpp
217

Your proposal is less powerful than the existing code as it's restricted to constants where the current code is didn't. Did you have a particular missed case in mind?

234

Hadn't been handled in the original patch, see forthcoming refresh.

300

I think nlopes question was actually about one of the operands being directly poison. Which is handled via the propagatesFullPoison code.

reames updated this revision to Diff 216464.Wed, Aug 21, 1:35 PM

Address the out of bounds base point brought up in review.

jdoerfert added inline comments.Wed, Aug 21, 3:52 PM
lib/Transforms/Instrumentation/PoisonChecking.cpp
217

I think, though I don't quite remember, I wanted to simplify the code. It was only a suggestion anyway.