[question] What's the correct way to disable FORTIFY_SOURCE?

[pdjonov]

I'm getting by with APP_CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=0 in Application.mk, which seems to work, but is that correct or is there a better/less-likely-to-break switch I should use instead?

It's breaking a whole bunch of code that does stuff like this:

ptr_to_buffer = static_cast<char*>(ptr_to_header_struct+1)
//or maybe with alignment if the buffer is something other than a string or some bytes

The compiler appears to decide that ptr_to_header_struct+1 is only a past-end pointer with respect to the header object and not also the pointer to the start of the next object. That turns any library call that can see where ptr_to_buffer came from into a checked call to manipulate a zero-length buffer, and the buffer is not zero-length. So, kaboom, crash.

Some of this is happening in libraries that require domain expertise to read, which I'm not qualified to rewrite so I can't just audit and fix all the code (which otherwise works wonderfully).

But even if I were to try, I'm not really sure what I'd have to do to make that pattern fly with Fortify. It is kind of a hard requirement in a number of places for performance reasons that I be able to find the next/previous object in a sequence where I know they're allocated back-to-back by doing a bit of pointer math, even if the standard is pedantic about the difference between past-end-of-prev and at-start-of-next pointers. Sorry if this is a redundant question. I have no idea what search terms to even start with on this question since every search with FORTIFY_SOURCE in it either brings up trivially broken code (like memset(&ptr, 0, sizeof(TPointedTo))) or a note in a discussion here about how FORTIFY_SOURCE's analysis produces no false positives which, uh, I'd like to dispute.

Thanks.

[enh-google]

a note in a discussion here about how FORTIFY_SOURCE's analysis produces no false positives which, uh, I'd like to dispute.

do you have a concrete example for @gburgessiv to look at?

[pdjonov]

Not sure what you're asking for exactly.

If you mean the comment, it's this one: #976 (comment).

If you mean an example of code that works if not for Fortify, I mean, I can't hand you my entire project. But one of the first breaks I noticed in the libraries we use is in Lua 5.2's garbage collector, which is publicly available and open source. One specific break is this bit from traversetable in lgc.c:

const TValue *mode = gfasttm(g, h->metatable, TM_MODE);
markobject(g, h->metatable);
if (mode && ttisstring(mode) &&  /* is there a weak mode? */
    ((weakkey = strchr(svalue(mode), 'k')),
     (weakvalue = strchr(svalue(mode), 'v')),
     (weakkey || weakvalue))) {  /* is really weak? */

After expanding macros and typedefs, that looks vaguely like this:

struct TValue {
  union {
    GCObject *gc;    /* collectable objects */
    void *p;         /* light userdata */
    int b;           /* booleans */
    lua_CFunction f; /* light C functions */
    double n;
  } value_;
  int tt_;
};

union GCObject {
  GCheader gch;  /* common header */
  union TString ts;
  union Udata u;
  union Closure cl;
  struct Table h;
  struct Proto p;
  struct UpVal uv;
  struct lua_State th;  /* thread */
};

struct GCHeader {
  GCObject *next;
  unsigned char tt;
  unsigned char marked;
};

union TString {
  union { double u; void *s; long l; } force_max_align;
  struct {
    GCObject *next;
    unsigned char tt;
    unsigned char marked;    
    unsigned char extra;
    unsigned int hash;
    size_t len;
  } tsv;
};

weakkey = strchr((const char*)((&mode->value_.gc->ts) +1), 'k') //<< kaboom

Digging into what strchr does with that expression, it seems to decide that the size of the buffer pointed to by (const char*)((&mode->value_.gc->ts) +1) is zero (which makes sense if you assume that's strictly a past-end pointer for ts and not also a pointer to the start of the next contiguous object), at which point it crashes the process since a null-terminated string can't possibly have zero storage size and it makes no sense to attempt to scan such storage for a character.

This pattern is everywhere in Lua's runtime and in a few other libs we use, as well as our own code. As I said, I'm not able to confidently rewrite all the libs that do this, and even in my own code I'm not sure how to make these pointer-offset shenanigans work "correctly" (given that, yes, we're by necessity in sketchy territory as far as a strict reading of the C/C++ specs goes).

[pdjonov]

Another thought: a call going into __strchr_chk with __n = 0 (which I'm pretty sure is what I was seeing in the debugger for the case above, though maybe lldb was just telling lies about optimized code and the issue was something even subtler) should probably be a compile-time warning (error?) rather than only a runtime assert.

[gburgessiv]

sorry for the delay.

I'm getting by with APP_CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=0 in Application.mk, which seems to work, but is that correct or is there a better/less-likely-to-break switch I should use instead?

-U_FORTIFY_SOURCE... should be enough on its own, but yeah, setting it forcibly to 0 is intended to turn it off, too. Depending on the patterns you're seeing, =1 might be a good middle-ground, since it makes FORTIFY less presumptuous about the memory available around field accesses in some cases.

Overall though, you're right. We have peepholes built into clang to try to be ultra-conservative in cases where the source has hints that users are overallocating storage on purpose (e.g., struct string { size_t len; char data[0]; };), but FORTIFY assumes that, in general, users won't try to jump between structs with pointer math trickery. In part due to standard pedantry, and in part because doing so is pretty rare in practice.

Perhaps more interestingly, FORTIFY sometimes becomes more aggressive when field accesses are textually present (which is the primary difference between =1 and =2); for example,

struct F {
  char cs[8];
  int64_t x;
};

void foo(F f) {
  char buf[16];
  strncpy(buf, f.cs, 9); // technically fine, sure. what the user intended? unlikely.
}

You can totally get FORTIFY to yell about code that's technically not broken, so if there are docs that say that FORTIFY never flags things that would turn into references of available memory at run-time, it'd be good to fix those.

[pdjonov]

Thanks for the reply!

I was worried that what I'd done was just an ugly hack and likely to break other stuff, but if that's the correct off switch and it isn't just quietly breaking something else then I'm good to go.

...and in part because doing so is pretty rare in practice.

Hah! Not in my line of work.

You can totally get FORTIFY to yell about code that's technically not broken...

It's possible I'm just bad at googling for things.

Documentation wise, I really struggled to find any sort of detailed information about what Fortify is doing at low level, what the likely pitfalls are, how to mitigate them, and what to do if I just need to turn it off (even if only to test whether it's the problem to being with - for all I knew initially Fortify had caught a heap corruption of some kind). The stuff I did find was either hyping up the benefits or difficult to parse discussions among compiler and library devs. I didn't work out what had gone wrong in our project until I'd sat here staring for quite a while at a message from Fortify that said something that suggested "hey I saved you from a buffer overrun" while the debugger clearly showed that the string I want to scan - null terminator and all - is correctly stored in memory at the address I just passed to strchr which ruled out memory corruption and led me to start disassembling code and trying to work backwards to infer what the compiler had done.

But again, thank you, you've not only answered my question but also confirmed that I'm still sane. Appreciate it. 😀

[YueAce99]

tkx you two，this helps me