On Sat, Sep 12, 2009 at 7:08 PM, Henri Verbeet hverbeet@gmail.com wrote:
2009/9/13 Ben Klein shacklein@gmail.com:
2009/9/13 Nicolas Le Cam niko.lecam@gmail.com:
Last one is also a false positive, it's just two pointers being subtracted to retrieve an offset.
That's not the reason why it's a false positive. Without context that line does look like a NULL-dereference (dmW is dereferenced to get the first pointer before any NULL checking). However it's in a static function that is called in two places, both of which already check the pointer being dereferenced.
No, that's not how it works. The expression "dmW->dmFormName" by itself doesn't dereference anything, it's just an address. Only once you read or write something from/to that address does dmW being NULL (or otherwise invalid) become a problem. Here we just subtract "dmW" from that address to get the field offset of "dmFormName" in the DEVMODEW structure. Note that the value of "off_formname" doesn't depend on the value of "dmW", just on its type. The compiler will probably recognize that and optimize it away. Arguably the code in question should just use the FIELD_OFFSET macro though.
Actually it does dereference something, if you think of dmFormName being an int (not a pointer), then you would be subtracting an address from a random value. If this being an offset is indeed the intent of this code, you need a & in front of dmW->dmFormName so the expression would read
ptrdiff_t off_formname = (const char *)(&dmW->dmFormName) - (const char *)dmW;
But as Ben noted, the cleaner way would be to use FIELD_OFFSET, which does exactly the above.
As for the context note, it is perfectly valid code (segfault-less, that is) as it stands, but we should either remove the null check on the next line or assign the value later.
Mike.