On 12/16/2014 02:43 AM, Yury Gribov wrote:> On 12/15/2014 10:24 PM, Ismail Pazarbasi wrote: >> s.d = 42.0; >> if (s.l > 100) // fire here > > Note that code like this is frequently used to convert integers to > floats so you'll get tons of false positives.True positives. The fix is to use memcpy instead. Emitting error for> accessing differently sized elements of enum may work (but should > already be handled by MSan?). > >> I have a few questions regarding the overall design: >> 1. Do you think this is a useful check? > > That's actually an interesting questions. It could be useful for tagged > unions although I believe programmers usually surround them with > checking asserts anyway.Useful, yes. It will find bugs. I haven't heard anyone clamoring for it though.>> 2. Where can I store type and field info about the union; some form of >> a shadow memory or a simple array/map? > > Without shadow it may be unacceptably slow in union-intensive > applications. But with shadow, it'll greatly complicate UBSan.None of the other checks in UBSan change the ABI. You can freely link ubsan .o files with non-ubsan .o files and the program will still work and ubsan will generate no false positives. With this check, that is not so. Uninstrumented code changing the active member of a union will cause a false positive when we next read it in instrumented code.>> 3. Should sanitizer abort immediately or continue upon detection? > > AFAIK normally UBSan checks continue after error (but there's a flag to > alter this). > >> 4. When/how can I remove entries from ubsan shadow memory when union's >> lifetime ends; perhaps in a module pass or at the end of each >> function? > > Take a look at how ASan does this (it's not easy). > > -Y > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >
On 12/19/2014 01:14 AM, Nick Lewycky wrote:> On 12/16/2014 02:43 AM, Yury Gribov wrote: >> On 12/15/2014 10:24 PM, Ismail Pazarbasi wrote: >>> s.d = 42.0; >>> if (s.l > 100) // fire here >> >> Note that code like this is frequently used to convert integers to >> floats so you'll get tons of false positives. > > True positives. The fix is to use memcpy instead.Hm, I thought C aliasing rules explicitly allow changing types through unions. Anyway, the pattern is so widespread that IMHO most maintainers will find such errors useless. -Y
Richard Smith
2014-Dec-20 02:59 UTC
[LLVMdev] [cfe-dev] ubsan - active member check for unions
On Thu, Dec 18, 2014 at 11:05 PM, Yury Gribov <y.gribov at samsung.com> wrote:> > On 12/19/2014 01:14 AM, Nick Lewycky wrote: > >> On 12/16/2014 02:43 AM, Yury Gribov wrote: >> >>> On 12/15/2014 10:24 PM, Ismail Pazarbasi wrote: >>> >>>> s.d = 42.0; >>>> if (s.l > 100) // fire here >>>> >>> >>> Note that code like this is frequently used to convert integers to >>> floats so you'll get tons of false positives. >>> >> >> True positives. The fix is to use memcpy instead. >> > > Hm, I thought C aliasing rules explicitly allow changing types through > unions.See my previous email; the kindest thing I can say about how C treats aliasing through unions is that it is confused.> Anyway, the pattern is so widespread that IMHO most maintainers will find > such errors useless.Well, given that this is the bug that the sanitizer is built to detect, such maintainers should not turn it on. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141219/1ae013e6/attachment.html>