[PATCH] D36810: Minimal runtime for UBSan.

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

In https://reviews.llvm.org/D36810#844964, @eugenis wrote:

> Good questions.
>
> The primary motivation is security. I want this to be something that I can be confident does not add any new vulnerabilities to the program. The current implementation demangles potentially untrusted strings, writes to arbitrary files, and in general contains too much code of mixed quality.
>
> In https://reviews.llvm.org/D36810#844863, @vsk wrote:
>
> > Have you explored the possibility of reducing the overhead of the regular runtime to an acceptable level?
> >
> > - If it's too big, we can disable any heavy weight features you identify at compile-time.
>
>
> Not worried about that.
>
> > - If it allocates or prints too much, we can add in a custom allocator or printing strategy.
> > - If it requires too much metadata to be inserted into user programs, we can devise a new, smaller encoding for the metadata. Or simply teach the runtime handlers to accept `nullptr`. This should also achieve the binary size savings you mentioned.
>
> Both metadata and extra code size to set up the call frame. UBSan checks are very common, and each instruction counts. If we are going to allow nullptr in the arguments, might as well implement _minimal variants.


I see, yes, this is a good reason to introduce the _minimal variants.

> 
> 
>> I have a strong preference towards having a single codebase for the runtime. An entirely separate runtime would impose a higher testing and maintenance burden. If having a separate runtime is really the only path forward, I would like to learn more about why the current runtime cannot be adapted to suit your use cases.
> 
> I'd love to have a single code base, but it creates lots of unnecessary complications. I thought of doing some sort of template or preprocessor magic to reuse the ubsan handlers, stripping out things like suppressions, argument processing, fancy output (demangling in particular, but also logging), UBSAN_OPTIONS handling when building as "minimal runtime". I feel the result would be a terrible mess.
> 
> The minimal runtime does not really need much testing - it's only 50 lines of code. It would be good to add something to ensure that the set of handlers matches the full runtime.

Fair enough. Having some way to enforce parity between the regular/minimal handlers would alleviate most of my concerns. Thanks for the explanation!



================
Comment at: compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cc:24
+#define HANDLER(name, msg)                                 \
+  INTERFACE void __ubsan_handle_##name##_minimal() {       \
+    message("ubsan: " msg "\n");                           \
----------------
eugenis wrote:
> vsk wrote:
> > eugenis wrote:
> > > pcc wrote:
> > > > There's a function type mismatch here I believe. If the idea is to reduce binary size, I'd imagine that we'd want to avoid passing the source location on the clang side.
> > > Good point. Completely forgot about that :)
> > I imagine that deduplication needs to be supported by a minimal runtime, to prevent e.g a tight for loop from spamming the UB log. How do you plan on accomplishing that without source locations -- perhaps by emitting a bitmap?
> Primary mode for this runtime, as a security hardening tool, would be abort-on-error. I even considered skipping the non-abort variants of the handlers.
> 
> For the sake of simplicity, we could do this with a plain array of caller PCs  which each handler will do a linear scan of, and probably not even reallocate it once it's full but just give up and print "error limit reached".
> 
Sgtm, this reduces the amount of inserted metadata further.


https://reviews.llvm.org/D36810