On Sat, Sep 12, 2009 at 7:08 PM, Henri Verbeet <[email protected]> wrote: > 2009/9/13 Ben Klein <[email protected]>: >> 2009/9/13 Nicolas Le Cam <[email protected]>: >>> 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.
