Sept 11 with tools and tests removed

[Mike Kaplinskiy]

On Sat, Sep 12, 2009 at 7:08 PM, Henri Verbeet <hverbeet at gmail.com> wrote:
> 2009/9/13 Ben Klein <shacklein at gmail.com>:
>> 2009/9/13 Nicolas Le Cam <niko.lecam at 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.