New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

work around _FORTIFY_SOURCE false positive #250

Merged
merged 1 commit into from Aug 4, 2015

Conversation

Projects
None yet
2 participants
@thestinger
Contributor

thestinger commented Aug 1, 2015

In builds with profiling disabled (default), the opt_prof_prefix array
has a one byte length as a micro-optimization. This will cause the usage
of write in the unused profiling code to be statically detected as a
buffer overflow by Bionic's _FORTIFY_SOURCE implementation as it tries
to detect read overflows in addition to write overflows.

@jasone

This comment has been minimized.

Member

jasone commented Aug 4, 2015

I prefer to leave this alone, since the warning does not indicate an exercisable issue, and the solution adds cpp noise. The cassert() makes the code unreachable in debug builds; can the static analyzer be set up to analyze a debug configuration?

@thestinger

This comment has been minimized.

Contributor

thestinger commented Aug 4, 2015

_FORTIFY_SOURCE is a library feature based on GCC intrinsics for hardening release builds (it actually depends on optimization) so I'm not really sure how else to approach it. It's true that it wouldn't trigger if debugging was enabled because the body of the function would be optimized out as abort is marked with __attribute__((noreturn)).

The error / warning is produced by __attribute__((error(...))) when the compiler fails to optimize out the static check, which is guarded by __builtin_constant_p to eliminate most false positives. It does the check at runtime via __write_chk if it knows the buffer size but not the size at compile-time and that wouldn't be a problem.

This doesn't happen with glibc because they don't check for buffer read overflows, only writes, and they use __attribute__((warning(...))) rather than __attribute__((error(...))).

@thestinger

This comment has been minimized.

Contributor

thestinger commented Aug 4, 2015

Other operating systems will probably be checking for read overflows too, so that was my motivation for submitting it here rather than just for Android's jemalloc tree. FreeBSD is getting a _FORTIFY_SOURCE implementation via GSoC but it doesn't look like it checks for overflows in write yet (but it's mentioned in the table as w*)

https://wiki.freebsd.org/SummerOfCode2015/FreeBSDLibcSecurityExtensions

@thestinger

This comment has been minimized.

Contributor

thestinger commented Aug 4, 2015

Oh, I have an idea. The cassert macro could be extended like this:

#define cassert(c) do {                         \
    if (unlikely(!(c)))                     \
        not_reached();                      \
    if (__builtin_constant_p((c)) && !(c))  \
        abort()
} while (0)

This would result in the function body being optimized out in release builds too, without adding the overhead of a branch anywhere as __builtin_constant_p will evaluate to false if the compiler can't figure it out at compile-time.

@thestinger

This comment has been minimized.

Contributor

thestinger commented Aug 4, 2015

(just need a __builtin_constant_p definition for non-GCC / clang compilers expanding to false)

@jasone

This comment has been minimized.

Member

jasone commented Aug 4, 2015

Cool, I'm all for a cassert() improvement if you can come up with something portable.

@thestinger

This comment has been minimized.

Contributor

thestinger commented Aug 4, 2015

This works well, and reduces the size of the shared object from 403k -> 367k.

@jasone

This comment has been minimized.

Member

jasone commented Aug 4, 2015

This is awesome. I have some thoughts and questions though.

  • clang appears to support __builtin_constant_p as well, so I think we need an autoconf feature test to make this change more broadly effective.
  • I actually like the idea of using __builtin_unreachable rather than abort(); my thinking is that undefined behavior is an equally valid failure mode if unreachable code is reached.
  • Can we do this in not_reached() rather than cassert() without introducing surprising side effects? Maybe the __builtin_unreachable needs to be non-debug-only?
@jasone

This comment has been minimized.

Member

jasone commented Aug 4, 2015

Hmm, __builtin_expect needs a feature test too. I can take care of these details if you don't want to.

@thestinger

This comment has been minimized.

Contributor

thestinger commented Aug 4, 2015

clang appears to support __builtin_constant_p as well, so I think we need an autoconf feature test to make this change more broadly effective.

Clang actually has nice built-ins for dealing with this without autoconf now (#if __has_builtin(__builtin_constant_p)).

http://clang.llvm.org/docs/LanguageExtensions.html#feature-checking-macros

It sets __GNUC__ even with -std=c99 though, so it's not necessary in this case as __builtin_constant_p has been around for years. For the same reason, __builtin_expect doesn't really need this either. They pretend to be GCC because doing anything else would make Clang perform poorly on existing code relative to GCC. It's only problematic when you want to use a GCC feature that's not in Clang.

I actually like the idea of using __builtin_unreachable rather than abort(); my thinking is that undefined behavior is an equally valid failure mode if unreachable code is reached.

It would make sense, but it's only a small code size win for dead code. I'm not sure if it's worth doing. I don't really like the idea of an incorrect assert like cassert(ptr != NULL) resulting in NULL checks following it being eliminated, but without actually performing the check itself.

The constant_p usage means it's either going to statically abort or not abort (no runtime overhead) so it's just a tiny code size difference in dead code. The __builtin_unreachable intrinsic is mostly useful for communicating invariants to trigger optimizations.

Can we do this in not_reached() rather than cassert() without introducing surprising side effects? Maybe the __builtin_unreachable needs to be non-debug-only?

It would make sense to use __builtin_unreachable at the bottom of not_reached (i.e. after the config_debug abort). It wouldn't be able to replace the static branch in cassert though.

@thestinger

This comment has been minimized.

Contributor

thestinger commented Aug 4, 2015

Ah, cassert is only used for the configuration stuff though - I forgot about that. It's a lot less risky than it seemed at first.

@thestinger

This comment has been minimized.

Contributor

thestinger commented Aug 4, 2015

@jasone: It actually optimizes out properly with just __builtin_unreachable in not_reached as you suggested.

work around _FORTIFY_SOURCE false positive
In builds with profiling disabled (default), the opt_prof_prefix array
has a one byte length as a micro-optimization. This will cause the usage
of write in the unused profiling code to be statically detected as a
buffer overflow by Bionic's _FORTIFY_SOURCE implementation as it tries
to detect read overflows in addition to write overflows.

This works around the problem by informing the compiler that
not_reached() means code in unreachable in release builds.

@jasone jasone merged commit 67c46a9 into jemalloc:dev Aug 4, 2015

@jasone

This comment has been minimized.

Member

jasone commented Aug 4, 2015

This change makes me really happy. Thanks again!

@thestinger thestinger deleted the thestinger:fortify_source branch Aug 4, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment