[PATCH] D37597: [ubsan] Function Sanitizer: Don't require writable text segments

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:445
+  auto *GV = new llvm::GlobalVariable(CGM.getModule(), Addr->getType(),
+                                      /*isConstant=*/false,
+                                      llvm::GlobalValue::PrivateLinkage, Addr);
----------------
vsk wrote:
> pcc wrote:
> > This can be constant I think.
> Can the value of the global change if a dynamic library is unloaded? I'm unsure about this case: A.dylib and B.dylib both have linkonce_odr definitions of X, dyld picks the definition from A, and A is unloaded.
I don't think it is possible to get into a situation where a global's address can change if a library is unloaded. In your scenario, suppose that B takes the address of X and then A is dlclosed. In that scenario the program would rightly expect the address to continue to be valid; to do otherwise would break the C++ standard's guarantee that each global has a unique address. That I believe is why POSIX does not provide a mechanism to require that a DSO is unloaded, only to request it to be unloaded; see the specification of dlclose [0].

The Linux manpage for dlclose is more specific: "If the reference  count  drops to zero and no other loaded libraries use symbols in it, then the dynamic library is unloaded." I believe that any references to X in B would count as a use of X in A.

[0] http://pubs.opengroup.org/onlinepubs/009695399/functions/dlclose.html


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:463
+  auto *PCRelAsInt =
+      Builder.CreatePtrToInt(EncodedAddr, IntPtrTy, "encoded_addr.int");
+  auto *FuncAsInt = Builder.CreatePtrToInt(F, IntPtrTy, "func_addr.int");
----------------
vsk wrote:
> vsk wrote:
> > pcc wrote:
> > > pcc wrote:
> > > > Maybe use `Int32Ty` (here and below). That should be sufficient under the small code model.
> > > Sorry, I meant that the difference could be truncated to `Int32Ty`, and stored as an integer, not a pointer.
> > I tried this out but it resulted in a mysterious dyld crash after the indirect callee returns, which I've yet to understand. I'll have to try using a Debug build of dyld to see what's happening.
> @pcc It turned out that the jump we encode in getUBSanFunctionSignature not correct if we truncate a 64-bit address to a 32-bit address. If we jump to +8 instead of +12, we can make the truncation work :). There is no dyld bug.
> 
> That said, I think we should store the pc-rel address as an IntPtrTy to keep things a bit simpler. We could avoid logic to truncate/sign-extend conditionally, and not emit those instructions at all. Wdyt?
The problem is that COFF does not have a relocation that can express a 64-bit relative reference. If we want something that will work with all object formats, 32-bit is probably best.


https://reviews.llvm.org/D37597