cppcheck sept 18 redux
False positive ticket created for: [/home/cahrendt/wine-git/dlls/wineps.drv/init.c:270]: (error) Possible null pointer dereference: dmW - otherwise it is redundant to check if dmW is null at line 272 what about the others? or the suggestion of adding an additional int to the array? chris
2009/9/21 chris ahrendt <celticht32(a)yahoo.com>:
False positive ticket created for:
[/home/cahrendt/wine-git/dlls/wineps.drv/init.c:270]: (error) Possible null pointer dereference: dmW - otherwise it is redundant to check if dmW is null at line 272
what about the others? or the suggestion of adding an additional int to the array?
The code is not using the ca[5] local variable, but the ca[1] defined in the cs_t struct. The question remains, how exactly does FIELD_OFFSET work, and does it end up dereferencing ca[5]? Furthermore, since this is in a test, is this expected in the first place? :)
Ben Klein wrote:
The question remains, how exactly does FIELD_OFFSET work, and does it end up dereferencing ca[5]? It does pointer arithmetic and does not dereference anything. "ca[5]" is the same as "(ca + 5)" or on lower level "((char*)ca + 5*sizeof(ca[0]))" and does not require any dereferencing.
[/home/cahrendt/wine-git/dlls/wineps.drv/init.c:270]: (error) Possible null pointer dereference: dmW - otherwise it is redundant to check if dmW is null at line 272
This is a real bug and should be fixed:
ptrdiff_t off_formname = (const char *)dmW->dmFormName - (const char *)dmW;
Does indeed dereference dmW to get the value of dmFormName. Vitaliy.
On Tue, Sep 22, 2009 at 1:09 AM, Vitaliy Margolen <wine-devel(a)kievinfo.com> wrote:
Ben Klein wrote:
The question remains, how exactly does FIELD_OFFSET work, and does it end up dereferencing ca[5]? It does pointer arithmetic and does not dereference anything. "ca[5]" is the same as "(ca + 5)" or on lower level "((char*)ca + 5*sizeof(ca[0]))" and does not require any dereferencing.
It does, since field offset macro takes the easy approach: #define FIELD_OFFSET(type, field) ((LONG)(INT_PTR)&(((type *)0)->field)) which basically dereferences a null pointer to get the offset. This would be a bug in cppcheck since we don't actually dereference ca[5]. Moreover, since cppcheck doesn't catch the similar FIELD_OFFSET uses as bugs, it seems that it is mistaking ca[5] for the local ca, as opposed to the cs_t->ca.
[/home/cahrendt/wine-git/dlls/wineps.drv/init.c:270]: (error) Possible null pointer dereference: dmW - otherwise it is redundant to check if dmW is null at line 272
This is a real bug and should be fixed:
ptrdiff_t off_formname = (const char *)dmW->dmFormName - (const char *)dmW; Does indeed dereference dmW to get the value of dmFormName.
It actually doesn't, it's a tricky case where dmW->dwFormName == &dmW->dwFormName, because dwFormName is an array allocated as part of the struct. I made that mistake too the previous cppcheck round.
Vitaliy.
Mike.
2009/9/22 Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com>:
On Tue, Sep 22, 2009 at 1:09 AM, Vitaliy Margolen <wine-devel(a)kievinfo.com> wrote:
[/home/cahrendt/wine-git/dlls/wineps.drv/init.c:270]: (error) Possible null pointer dereference: dmW - otherwise it is redundant to check if dmW is null at line 272
This is a real bug and should be fixed:
ptrdiff_t off_formname = (const char *)dmW->dmFormName - (const char *)dmW; Does indeed dereference dmW to get the value of dmFormName.
It actually doesn't, it's a tricky case where dmW->dwFormName == &dmW->dwFormName, because dwFormName is an array allocated as part of the struct. I made that mistake too the previous cppcheck round.
I thought I'd covered this before. It's complaining about dmW not being validated before dmW->dmFormName (which de-references dmW to get at its dmFormName member). In current code, it's not a problem because dmW is validated before it's run in both cases (lines 369 and 403). It is right about the redundant test NULL check on 272 though; if dmW is NULL, the function will segfault.
On Tue, Sep 22, 2009 at 3:06 AM, Ben Klein <shacklein(a)gmail.com> wrote:
2009/9/22 Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com>:
On Tue, Sep 22, 2009 at 1:09 AM, Vitaliy Margolen <wine-devel(a)kievinfo.com> wrote:
[/home/cahrendt/wine-git/dlls/wineps.drv/init.c:270]: (error) Possible null pointer dereference: dmW - otherwise it is redundant to check if dmW is null at line 272
This is a real bug and should be fixed:
ptrdiff_t off_formname = (const char *)dmW->dmFormName - (const char *)dmW; Does indeed dereference dmW to get the value of dmFormName.
It actually doesn't, it's a tricky case where dmW->dwFormName == &dmW->dwFormName, because dwFormName is an array allocated as part of the struct. I made that mistake too the previous cppcheck round.
I thought I'd covered this before. It's complaining about dmW not being validated before dmW->dmFormName (which de-references dmW to get at its dmFormName member). In current code, it's not a problem because dmW is validated before it's run in both cases (lines 369 and 403). It is right about the redundant test NULL check on 272 though; if dmW is NULL, the function will segfault.
It actually does not dereference anything. Try passing null into the function - it will work just fine. This is a special case because the array isn't dynamically allocated but is part of the struct, which means that dmW->dmFormName == (dmW+__offset of dmFormName) and not *(dmW+__offset of dmFormName). You can try writing a test program yourself - it will run just fine. Mike.
2009/9/22 Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com>:
On Tue, Sep 22, 2009 at 3:06 AM, Ben Klein <shacklein(a)gmail.com> wrote:
2009/9/22 Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com>:
On Tue, Sep 22, 2009 at 1:09 AM, Vitaliy Margolen <wine-devel(a)kievinfo.com> wrote:
[/home/cahrendt/wine-git/dlls/wineps.drv/init.c:270]: (error) Possible null pointer dereference: dmW - otherwise it is redundant to check if dmW is null at line 272
This is a real bug and should be fixed:
ptrdiff_t off_formname = (const char *)dmW->dmFormName - (const char *)dmW; Does indeed dereference dmW to get the value of dmFormName.
It actually doesn't, it's a tricky case where dmW->dwFormName == &dmW->dwFormName, because dwFormName is an array allocated as part of the struct. I made that mistake too the previous cppcheck round.
I thought I'd covered this before. It's complaining about dmW not being validated before dmW->dmFormName (which de-references dmW to get at its dmFormName member). In current code, it's not a problem because dmW is validated before it's run in both cases (lines 369 and 403). It is right about the redundant test NULL check on 272 though; if dmW is NULL, the function will segfault.
It actually does not dereference anything. Try passing null into the function - it will work just fine. This is a special case because the array isn't dynamically allocated but is part of the struct, which means that dmW->dmFormName == (dmW+__offset of dmFormName) and not *(dmW+__offset of dmFormName). You can try writing a test program yourself - it will run just fine.
OK, yes, that's very weird, but you're right. Definitely a bug in cppcheck then, either way.
Mike Kaplinskiy wrote:
It actually does not dereference anything. Try passing null into the function - it will work just fine. This is a special case because the array isn't dynamically allocated but is part of the struct, which means that dmW->dmFormName == (dmW+__offset of dmFormName) and not *(dmW+__offset of dmFormName). You can try writing a test program yourself - it will run just fine. It does dereference the pointer. Here is your simple test. Compile it and run it. See what happens.
#include <stdio.h> typedef struct _s_test { void *pointer; } s_test; int main() { s_test *s = NULL; long diff = (const char*)s->pointer - (const char*)s; printf("diff=%ld\n", diff); return 0; } Vitaliy.
2009/9/22 Vitaliy Margolen <wine-devel(a)kievinfo.com>:
It does dereference the pointer. Here is your simple test. Compile it and run it. See what happens.
#include <stdio.h>
typedef struct _s_test { void *pointer; } s_test;
int main() { s_test *s = NULL; long diff = (const char*)s->pointer - (const char*)s; printf("diff=%ld\n", diff);
return 0; }
Vitaliy.
That's not really what the code does. Try changing "void *pointer" to "char pointer[32]".
Henri Verbeet wrote:
2009/9/22 Vitaliy Margolen <wine-devel(a)kievinfo.com>:
It does dereference the pointer. Here is your simple test. Compile it and run it. See what happens.
#include <stdio.h>
typedef struct _s_test { void *pointer; } s_test;
int main() { s_test *s = NULL; long diff = (const char*)s->pointer - (const char*)s; printf("diff=%ld\n", diff);
return 0; }
Vitaliy.
That's not really what the code does. Try changing "void *pointer" to "char pointer[32]". Right, haven't checked the definition of the DEVMODEW. Indeed it does work for an array, since it is part of the structure. So "s->pointer" will be (char *)s + FIELD_OFFSET(s,pointer).
Still it's a bad construct to use, for exactly the reason we are having this discussion. Vitaliy.
2009/9/22 Vitaliy Margolen <wine-devel(a)kievinfo.com>:
Mike Kaplinskiy wrote:
It actually does not dereference anything. Try passing null into the function - it will work just fine. This is a special case because the array isn't dynamically allocated but is part of the struct, which means that dmW->dmFormName == (dmW+__offset of dmFormName) and not *(dmW+__offset of dmFormName). You can try writing a test program yourself - it will run just fine. It does dereference the pointer. Here is your simple test. Compile it and run it. See what happens.
#include <stdio.h>
typedef struct _s_test { void *pointer;
No. Array, not pointer. E.g.: int array[1];
} s_test;
2009/9/22 Ben Klein <shacklein(a)gmail.com>:
2009/9/22 Vitaliy Margolen <wine-devel(a)kievinfo.com>:
Mike Kaplinskiy wrote:
It actually does not dereference anything. Try passing null into the function - it will work just fine. This is a special case because the array isn't dynamically allocated but is part of the struct, which means that dmW->dmFormName == (dmW+__offset of dmFormName) and not *(dmW+__offset of dmFormName). You can try writing a test program yourself - it will run just fine. It does dereference the pointer. Here is your simple test. Compile it and run it. See what happens.
#include <stdio.h>
typedef struct _s_test { void *pointer;
No. Array, not pointer. E.g.: int array[1];
} s_test;
If it IS the case that this doesn't cause a crash and is perfectly valid, can someone explain to me how/why this works? Or point me (no pun intended) to the bit in the C spec that explains it? Coz the way I read it, it has to dereference dmW, otherwise how would the compiler find the address of the array? ... so confused :) Luke.
2009/9/23 Luke Benstead <kazade(a)gmail.com>:
If it IS the case that this doesn't cause a crash and is perfectly valid, can someone explain to me how/why this works? Or point me (no pun intended) to the bit in the C spec that explains it? Coz the way I read it, it has to dereference dmW, otherwise how would the compiler find the address of the array? ... so confused :)
I believe it's because the array (as a pointer) is at the same location as start of the struct (as a pointer). Compiler then applies pointer arithmetic without dereferencing.
2009/9/22 Ben Klein <shacklein(a)gmail.com>:
2009/9/23 Luke Benstead <kazade(a)gmail.com>:
If it IS the case that this doesn't cause a crash and is perfectly valid, can someone explain to me how/why this works? Or point me (no pun intended) to the bit in the C spec that explains it? Coz the way I read it, it has to dereference dmW, otherwise how would the compiler find the address of the array? ... so confused :)
I believe it's because the array (as a pointer) is at the same location as start of the struct (as a pointer). Compiler then applies pointer arithmetic without dereferencing.
Ah, I see.. but in that case, how is an array different to using a pointer, like in Vitaliy's example? Surely that's the same thing essentially? Luke.
2009/9/22 Luke Benstead <kazade(a)gmail.com>:
2009/9/22 Ben Klein <shacklein(a)gmail.com>:
2009/9/23 Luke Benstead <kazade(a)gmail.com>:
If it IS the case that this doesn't cause a crash and is perfectly valid, can someone explain to me how/why this works? Or point me (no pun intended) to the bit in the C spec that explains it? Coz the way I read it, it has to dereference dmW, otherwise how would the compiler find the address of the array? ... so confused :)
I believe it's because the array (as a pointer) is at the same location as start of the struct (as a pointer). Compiler then applies pointer arithmetic without dereferencing.
Ah, I see.. but in that case, how is an array different to using a pointer, like in Vitaliy's example? Surely that's the same thing essentially?
"s->pointer" refers to the value of the pointer if "pointer" is a pointer, but to the address of the first array element if "pointer" is an array. I.e. "&s->pointer[0]" as Nicolas posted.
2009/9/22 Henri Verbeet <hverbeet(a)gmail.com>:
"s->pointer" refers to the value of the pointer if "pointer" is a pointer, but to the address of the first array element if "pointer" is an array. I.e. "&s->pointer[0]" as Nicolas posted.
To clarify a bit more, if it's an array the actual array elements are stored in the structure, so &s->pointer[0] is just the offset of the field in the array, but if it's a pointer just the address of whatever the pointer points to is stored in the structure, so you have to dereference the pointer to the structure to get to it.
On Tue, Sep 22, 2009 at 10:48 AM, Luke Benstead <kazade(a)gmail.com> wrote:
2009/9/22 Ben Klein <shacklein(a)gmail.com>:
2009/9/23 Luke Benstead <kazade(a)gmail.com>:
If it IS the case that this doesn't cause a crash and is perfectly valid, can someone explain to me how/why this works? Or point me (no pun intended) to the bit in the C spec that explains it? Coz the way I read it, it has to dereference dmW, otherwise how would the compiler find the address of the array? ... so confused :)
I believe it's because the array (as a pointer) is at the same location as start of the struct (as a pointer). Compiler then applies pointer arithmetic without dereferencing.
Ah, I see.. but in that case, how is an array different to using a pointer, like in Vitaliy's example? Surely that's the same thing essentially?
Luke.
It basically has to do with semantics. If you have ``char arr[5];'', the ``arr`` will refer to the address of the first element, ie ``arr==&arr''. The same applies for fixed size arrays in structs - because the array is allocated as part of the struct, ``dmW->dmFormName'' refers to the address of the 0th element (which as the compiler knows is ``dmW+__offset of dmFormName''), or as above ``dmW->dmFormName=&dmW->dmFormName''. For the pointer case (``char *arr;''), ``arr'' would be an actual pointer - that is, ``&arr != arr'', since it stores an address and has a location of its own (whereas fixed length arrays just have a location). This would make a nice interview question. Mike.
2009/9/22 Luke Benstead <kazade(a)gmail.com>:
2009/9/22 Ben Klein <shacklein(a)gmail.com>:
2009/9/22 Vitaliy Margolen <wine-devel(a)kievinfo.com>:
Mike Kaplinskiy wrote:
It actually does not dereference anything. Try passing null into the function - it will work just fine. This is a special case because the array isn't dynamically allocated but is part of the struct, which means that dmW->dmFormName == (dmW+__offset of dmFormName) and not *(dmW+__offset of dmFormName). You can try writing a test program yourself - it will run just fine. It does dereference the pointer. Here is your simple test. Compile it and run it. See what happens.
#include <stdio.h>
typedef struct _s_test { void *pointer;
No. Array, not pointer. E.g.: int array[1];
} s_test;
If it IS the case that this doesn't cause a crash and is perfectly valid, can someone explain to me how/why this works? Or point me (no pun intended) to the bit in the C spec that explains it? Coz the way I read it, it has to dereference dmW, otherwise how would the compiler find the address of the array? ... so confused :)
Luke.
Luke, Wine's current code is basically equivalent to the one above, where there's no dereference : #include <stdio.h> typedef struct _s_test { char pointer[5]; } s_test; int main() { s_test *s = NULL; long diff = (const char*)(&s->pointer[0]) - (const char*)s; printf("diff=%ld\n", diff); return 0; } -- Nicolas Le Cam
Luke Benstead skrev:
If it IS the case that this doesn't cause a crash and is perfectly valid, can someone explain to me how/why this works? Or point me (no pun intended) to the bit in the C spec that explains it? Coz the way I read it, it has to dereference dmW, otherwise how would the compiler find the address of the array? ... so confused :)
Kernighan & Ritchie, "The C Programming Language", 2nd edition, section 5.3, page 99: "Since the name of an array is a synonym for the location of the initial element, the assignment pa=&a[0] can also be written as pa=a" The reference part, section A7.1, goes into more detail. "If the type of an expression or subexpression is array of T, for some type T, then the value of the expression is a pointer to the first object in the array, and the type of the expression is altered to pointer to T." It then lists a few exceptions, including the & operator is used (hence, a and &a returns the exact same thing if a is an array).
On Tuesday 22 September 2009 12:32:35 am Mike Kaplinskiy wrote:
It actually does not dereference anything.
Does the C standard specify that taking the address of a struct member being dereferenced doesn't actually cause a dereference, instead just offsetting? Doing foo-> is identical to (*foo)., so dmW->dmFormName is the same as &(*dmW).dmFormName, which does technically cause a dereference, followed by taking the address of the field. However, since GCC will remove deadcode and it's simple to see the dereference isn't needed, it just optimizes it away. I wouldn't even be surprised if this behavior is guaranteed by GCC with no optimizations enabled.. but I'm not so sure that it's guaranteed by the C standard. Is it?
2009/9/23 Chris Robinson <chris.kcat(a)gmail.com>:
On Tuesday 22 September 2009 12:32:35 am Mike Kaplinskiy wrote:
It actually does not dereference anything.
Does the C standard specify that taking the address of a struct member being dereferenced doesn't actually cause a dereference, instead just offsetting? Doing foo-> is identical to (*foo)., so dmW->dmFormName is the same as &(*dmW).dmFormName, which does technically cause a dereference, followed by taking the address of the field.
However, since GCC will remove deadcode and it's simple to see the dereference isn't needed, it just optimizes it away. I wouldn't even be surprised if this behavior is guaranteed by GCC with no optimizations enabled.. but I'm not so sure that it's guaranteed by the C standard. Is it?
Worth trying my sample code on non-GCC compilers then: #include <stdio.h> #include <stdlib.h> struct foo { int baz[1]; }; int main() { struct foo *bar=NULL; printf("%p\n",(void*)(NULL)); printf("%p\n",(void*)(bar->baz)); printf("%p\n",(void*)((*bar).baz)); printf("%p\n",(void*)(&(*bar).baz[5])); printf("%d\n",(sizeof(int[5]))); return 0; } Expected output: (nil) (nil) (nil) 0x14 20 Is there a reason why sizeof() magic can't be used instead of FIELD_OFFSET? :)
On Tuesday 22 September 2009 7:53:08 am Ben Klein wrote:
Worth trying my sample code on non-GCC compilers then:
With all optimizations off (if I had non-GCC compilers, I'd try them). But still, I'm not really saying this is a problem (especially since the FIELD_OFFSET macro does this, and is used in many places throughout Wine) as long as the major compilers will handle it as expected. I'm just not so sure we should call the report a false positive, if the C standard doesn't specify that behavior.
2009/9/22 Chris Robinson <chris.kcat(a)gmail.com>:
On Tuesday 22 September 2009 12:32:35 am Mike Kaplinskiy wrote:
It actually does not dereference anything.
Does the C standard specify that taking the address of a struct member being dereferenced doesn't actually cause a dereference, instead just offsetting? Doing foo-> is identical to (*foo)., so dmW->dmFormName is the same as &(*dmW).dmFormName, which does technically cause a dereference, followed by taking the address of the field.
However, since GCC will remove deadcode and it's simple to see the dereference isn't needed, it just optimizes it away. I wouldn't even be surprised if this behavior is guaranteed by GCC with no optimizations enabled.. but I'm not so sure that it's guaranteed by the C standard. Is it?
I'm not sure if the standard makes any guarantees about that either, but the underlying machine code more or less does, since you don't have the concept of an address-of operator there.
Chris Robinson skrev:
On Tuesday 22 September 2009 12:32:35 am Mike Kaplinskiy wrote:
It actually does not dereference anything.
Does the C standard specify that taking the address of a struct member being dereferenced doesn't actually cause a dereference, instead just offsetting?
It doesn't specify that * causes a dereference as such. It's known as the "indirection" operator (not the "dereference" operator), and if its operand is a pointer, it "returns" the object pointed to, as a lvalue, which is no more a dereference than any other expression. Only evaluating or assigning to this lvalue would cause an actual access (or dereference). In other words, just like &some_var will not actually access some_var, &(*ptr_var) will not actually dereference ptr_var. (There'd be no point in doing that.)
Doing foo-> is identical to (*foo)., so dmW->dmFormName is the same as &(*dmW).dmFormName, which does technically cause a dereference, followed by taking the address of the field.
I doubt that'd even be possible. Dereferencing an address would take the data at that address, and you can't relate that data back to an address. Hence, the only sane way to take an address is to not dereference it first. (And what would the dereference be for? On most architectures I know, there's no way to tell the CPU to access memory without either reading or writing that memory location, and this code does neither.)
However, since GCC will remove deadcode and it's simple to see the dereference isn't needed, it just optimizes it away. I wouldn't even be surprised if this behavior is guaranteed by GCC with no optimizations enabled.. but I'm not so sure that it's guaranteed by the C standard. Is it?
I'm not sure, but I haven't heard about any "please try to emit meaningless code for no reason" section, so this kind of construct is a generally safe, and very, *very* widespread, practice.
On Di, 2009-09-22 at 07:46 -0700, Chris Robinson wrote:
so dmW->dmFormName is the same as &(*dmW).dmFormName,
When the target is an array, the address of the first member is returned: dmW->fmFormName is: &(dmW->dmFormName[0]) similar: char buffer[32]; printf("buffer at %p\n", buffer); which is only a shortcut for: printf("buffer[0] at %p\n", &buffer[0]);
However, since GCC will remove deadcode and it's simple to see the dereference isn't needed, it just optimizes it away. No, removing deadcode must be enabled. You can use every compiler.
but I'm not so sure that it's guaranteed by the C standard. Is it?
Yes. -- By by ... Detlef
On Tue, Sep 22, 2009 at 02:56:05AM -0400, Mike Kaplinskiy wrote:
On Tue, Sep 22, 2009 at 1:09 AM, Vitaliy Margolen <wine-devel(a)kievinfo.com> wrote:
Ben Klein wrote:
The question remains, how exactly does FIELD_OFFSET work, and does it end up dereferencing ca[5]? It does pointer arithmetic and does not dereference anything. "ca[5]" is the same as "(ca + 5)" or on lower level "((char*)ca + 5*sizeof(ca[0]))" and does not require any dereferencing.
It does, since field offset macro takes the easy approach: #define FIELD_OFFSET(type, field) ((LONG)(INT_PTR)&(((type *)0)->field))
which basically dereferences a null pointer to get the offset. This would be a bug in cppcheck since we don't actually dereference ca[5]. Moreover, since cppcheck doesn't catch the similar FIELD_OFFSET uses as bugs, it seems that it is mistaking ca[5] for the local ca, as opposed to the cs_t->ca.
I suspect that the above is technically illegal C. Mainly because pointer arithmetic is only defined for pointers to objects - and no object can have the address 0. This is why offsetof() is defined as a builtin in more recent GCC. The case in question tries to evaluate: ((LONG)(INT_PTR)&(((type *)0)->ca[5])) which does seem to contain a reference to beyond the end of the array! Modern C allows the last entry of a struct to be ca[] (ie no array bound) for these situations where a structure will be malloced with dynamic size. There is, however, a fubar in the standard. offsetof() is defined to return a compile-time constant - so the result can be used as an array size etc. However there are times when you want to do: offsetof(type, array_member[expression]) and this now reported as illegal by gcc unless 'espression' is a compile time constant :-( David -- David Laight: david(a)l8s.co.uk
participants (11)
-
Ben Klein -
chris ahrendt -
Chris Robinson -
David Laight -
Detlef Riekenberg -
Henri Verbeet -
Luke Benstead -
Mike Kaplinskiy -
Nicolas Le Cam -
Ove Kaaven -
Vitaliy Margolen