[PATCH] D33305: [ubsan] Add a check for pointer overflow UB

Vedant Kumar via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu May 18 09:38:34 PDT 2017


vsk added a comment.

Thanks for the comments, responses inline --



================
Comment at: lib/CodeGen/CGExprScalar.cpp:3854
+                                               const Twine &Name) {
+  Value *GEPVal = Builder.CreateInBoundsGEP(Ptr, IdxList, Name);
+
----------------
filcab wrote:
> You're creating the GEP first (possibly triggering UB), and then checking ("after" UB). Shouldn't you put the checking instructions before the GEP?
The checking instructions are not users of the (possibly poisoned) GEP value so inserting the checks after the GEP should be OK. FWIW ubsan's scalar range checks do the same thing and I haven't seen an issue there.

I do have a concern about the IR constant folder reducing GEPs to Undef, but I don't think this affects any interesting cases, and we handle that below by bailing out if the GEP is a Constant.

Does that clear up your concerns?


================
Comment at: lib/CodeGen/CGExprScalar.cpp:3948
+    return GEPVal;
+
+  // Now that we've computed the total offset, add it to the base pointer (with
----------------
filcab wrote:
> Do we want an extra test for `TotalOffset` being a constant + not overflowing? (Not strictly need, but you've been working on avoiding __ubsan() calls :-) )
If the total offset is a constant and doesn't overflow, there's no need for the select instruction below, but we'd still need to emit a pointer overflow check. Teaching clang to omit the select is a little messy, and I don't think we'd save very much, so I'm leaning against doing it.


https://reviews.llvm.org/D33305





More information about the cfe-commits mailing list