Your fix is very error prone, next time someone adds a
member to the fcode structure, it's very likely that the
initialization will be missing and will lead to yet
another bug.
memset has to be used to initialize structures,
initializing every single member is not robust and
increases codesize
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The next time someone adds a member to the fcode structure, it will have to
be set to the *CORRECT* value before the structure is used. There is *NO*
guarantee that the 0 that your memset call would set it to would be the
correct value, so the member should be set individually (even if the correct
value is 0, just so that it's clear that it *is* being set to 0).
Memset does *NOT* have to be used to initialize structures. Relying on it to
do all the structure initialization you need is also not robust, as I indicated,
and the code size increase is irrelevant.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
When a structure is used a thousand time and someone adds
a pointer to the structure, do you manually dig into the
code and initialize a thousand time this pointer ? There
is a very high probability that some structure will not be
initialized correctly.
With the memset solution, there is no need to do
*anything*.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
> When a structure is used a thousand time and someone adds
a pointer to the structure, do you manually dig into the
code and initialize a thousand time this pointer ?
If NULL is not a valid value for that pointer, yes, you would have to manually
change all of those thousand usages. The memset solution would set the
pointer to null (at least on platforms where a null pointer has all its bits zero,
which is true of all but some obscure platforms, but which is *NOT*
guaranteed by the C specification), so, if the pointer has to point to
something valid, there *would* be a need to do something with the memset
solution - there would be a need to make the pointer point to something
valid (and to arrange that said "something valid" exist).
Note also that not all structure fields are pointers, and that other fields might
well have to have a non-zero value.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Your fix doesn't even work, I told you that your solution
is error prone. USE MEMSET!!!
here is the valgrind output:
=20771== Thread 2:
==20771== Syscall param socketcall.setsockopt(optval)
points to uninitialised byte(s)
==20771== at 0x985B67: setsockopt (in /lib/libc-2.4.so)
==20771== by 0x402B8B8: pcap_setfilter (pcap.c:701)
==20771== by 0x804D2AC: capture_loop_init_filter
(in /home/heefre/software/dbus/wireshark/build/dumpcap)
==20771== by 0x804DBA2: capture_loop_start
(in /home/heefre/software/dbus/wireshark/build/dumpcap)
==20771== by 0x40215AC: sniffer_start_thread
(in /home/heefre/software/dbus/wireshark/build/lib/libcapdbus.so)
==20771== by 0x2008FE: g_thread_create_proxy
(gthread.c:582)
==20771== by 0xB41432: start_thread
(in /lib/libpthread-2.4.so)
==20771== by 0x984A1D: clone (in /lib/libc-2.4.so)
==20771== Address 0x4B4D0E2 is on thread 2's stack
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Are you certain that valgrind complained about *one* unitialized variable, not
*two*? The valgrind report does *not* indicate which line number in pcap-
linux.c made the setsockopt() call, perhaps because the call to p-
>setfilter_op in pcap_setfilter() was tail-call-optimized.
There are *multiple* setsockopt() calls in the setfilter code path.
There's a call with SO_ATTACH_FILTER and total_fcode as the filter argument.
total_fcode has static storage scope, so it would be uninitialized only if the C
compiler had a serious bug, so that's extremely unlikely to be the problematic
call.
Then there's the call with SO_ATTACH_FILTER and the fcode structure in
question. At least according to my copy of the Linux 2.6.17.3 source, "struct
sock_fprog" has two members, and both are set both by fix_program() if it
returns a value other than -1; the only code path where that call is made is
one where fix_program() returns 1 (if it returns -1, pcap_setfilter_linux()
returns before setting the filter; if it returns 0, it sets can_filter_in_kernel to 0
so pcap_setfilter_linux() doesn't try to set the kernel filter).
For that to cause an uninitialized reference, either:
1) the version of the kernel you're running would have to have added
another field to struct sock_fprog, in which case
1) they would have broken binary compatibility with all code using
SO_ATTACH_FILTER
and
2) without seeing their change, or a description of it, it cannot be
assumed that setting that field to 0 is the correct thing to do, so it cannot be
assumed that using memset will correctly fix the problem (i.e., using memset,
as I already explained, is not guaranteed to work, where "work" is defined as
"makes the code work correctly" rather than "makes valgrind not complain
about the code").
Then there's the call in reset_kernel_filter(); if there are errors setting the
kernel filter, that call is made to remove the "total filter". At least according
to the source code to valgrind 3.2.1, the check valgrind makes on the last two
arguments setsockopt() calls is that it checks that all N bytes pointed to by
the next-to-last argument are initialized, where N is the last argument; it
does not know, for example, that particular options, such as
SO_DETACH_FILTER, don't use their arguments. The dummy argument wasn't
initialized.
I've checked in a change to initialize the dummy argument to squelch the
valgrind complaint. If that's *not* the call that's causing the problem, then
either
1) an additional member has been added to the sock_fprog structure, in
which case we have to know what that member is used for before setting it
or
2) there's some code path where fix_program() isn't called, or returns -1,
but where the socket filter gets set, in which case *that's* the bug -
memsetting the structure merely papers over that bug.
Using memset here would be the equivalent of dealing with a cracked wall in
a building by sanding and re-painting the wall to cover up the crack, without
checking the structure of the building to see whether there are any serious
problems of which the crack is just a symptom.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
In this case where the structure does not contain pointer,
your complain about memset is invalid.
The kernel can add whatever member to the struture, memset
will protect the code from using uninitialized variable.
The code has to run on any kernel, past, present or
future, without any error. If tomorow, kernel developpers
add a member, the code will be broken if it doesn't use
memset
By the way, memset is used 9 times in pcap_linux.c to
initialize structure, do you consider these as mistakes
too ?
What are the name of platform that doesn't initialized
pointer to 0 ?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
> In this case where the structure does not contain pointer,
your complain about memset is invalid.
> The kernel can add whatever member to the struture, memset
will protect the code from using uninitialized variable.
It will not, however, protect the code from using an *incorrectly initialized*
variable, if the correct value for the structure member isn't 0.
> The code has to run on any kernel, past, present or
future, without any error. If tomorow, kernel developpers
add a member, the code will be broken if it doesn't use
memset
As I have already explained, several times, the code could be broken even if it
*does* use memset, if, for example, the SO_ATTACH_FILTER API changes so
that you have to supply a pointer to some other item in order to get the
desired behavior, or if you have to supply a filter type code of some sort
where the type desired is *not* 0, or if you have to supply a set of flags and
one of the flags has to be set to get the desired behavior.
> By the way, memset is used 9 times in pcap_linux.c to
initialize structure, do you consider these as mistakes
too ?
If the structure isn't defined by libpcap, and if the structure is subject to
change, yes, I do.
> What are the name of platform that doesn't initialized
pointer to 0 ?
Define "initialize pointer to 0". The C programming language specification
says that a constant value of 0 is, when assigned to or compared with a
pointer value, converted to a "null pointer constant", which is not guaranteed
to have all its bits zero. Thus:
Whether any C implementations have been done using those null pointer
representations is another matter, and it's probably quite unlikely that any
*interesting* platforms have that characteristic. I didn't add that as an
indication that it's worth worrying about whether memset(..., 0, ...) will
initialize pointers to 0 in libpcap, I just added it as a parenthetical note to
educate those who might think that C guarantees that all bits of a null pointer
are 0.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The solution is to use a contructor for the structure,
instead of using memset. Is there any contructor of the
fcode structure, I bet no. So a memset is the least worst
solution and acts a a shield to protect the application.
A value incorrectly initialized to 0 is definitly better
than an uninitialized value. At least a memset will not
make the application crashed
Using a constructor is better than memset, and memset is
better than individually initialized each member.
The 9 memset are almost all kernel structures.
When thinking about this problem, I'm so glad to have
moved to 00 programming, except of course when my app has
to link to a C library.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Administrators of the "libpcap" SourceForge project have superseded this tracker item (formerly artifact 1552777, now bug 75) with issue 77 of the "libpcap" GitHub project.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
libpcap-uninitialized-var-fcode.path
Logged In: YES
user_id=541179
I've checked in a different fix.
Logged In: YES
user_id=1320060
Your fix is very error prone, next time someone adds a
member to the fcode structure, it's very likely that the
initialization will be missing and will lead to yet
another bug.
memset has to be used to initialize structures,
initializing every single member is not robust and
increases codesize
Logged In: YES
user_id=541179
The next time someone adds a member to the fcode structure, it will have to
be set to the *CORRECT* value before the structure is used. There is *NO*
guarantee that the 0 that your memset call would set it to would be the
correct value, so the member should be set individually (even if the correct
value is 0, just so that it's clear that it *is* being set to 0).
Memset does *NOT* have to be used to initialize structures. Relying on it to
do all the structure initialization you need is also not robust, as I indicated,
and the code size increase is irrelevant.
Logged In: YES
user_id=1320060
When a structure is used a thousand time and someone adds
a pointer to the structure, do you manually dig into the
code and initialize a thousand time this pointer ? There
is a very high probability that some structure will not be
initialized correctly.
With the memset solution, there is no need to do
*anything*.
Logged In: YES
user_id=541179
> When a structure is used a thousand time and someone adds
a pointer to the structure, do you manually dig into the
code and initialize a thousand time this pointer ?
If NULL is not a valid value for that pointer, yes, you would have to manually
change all of those thousand usages. The memset solution would set the
pointer to null (at least on platforms where a null pointer has all its bits zero,
which is true of all but some obscure platforms, but which is *NOT*
guaranteed by the C specification), so, if the pointer has to point to
something valid, there *would* be a need to do something with the memset
solution - there would be a need to make the pointer point to something
valid (and to arrange that said "something valid" exist).
Note also that not all structure fields are pointers, and that other fields might
well have to have a non-zero value.
Logged In: YES
user_id=1320060
Your fix doesn't even work, I told you that your solution
is error prone. USE MEMSET!!!
here is the valgrind output:
=20771== Thread 2:
==20771== Syscall param socketcall.setsockopt(optval)
points to uninitialised byte(s)
==20771== at 0x985B67: setsockopt (in /lib/libc-2.4.so)
==20771== by 0x402B8B8: pcap_setfilter (pcap.c:701)
==20771== by 0x804D2AC: capture_loop_init_filter
(in /home/heefre/software/dbus/wireshark/build/dumpcap)
==20771== by 0x804DBA2: capture_loop_start
(in /home/heefre/software/dbus/wireshark/build/dumpcap)
==20771== by 0x40215AC: sniffer_start_thread
(in /home/heefre/software/dbus/wireshark/build/lib/libcapdbus.so)
==20771== by 0x2008FE: g_thread_create_proxy
(gthread.c:582)
==20771== by 0xB41432: start_thread
(in /lib/libpthread-2.4.so)
==20771== by 0x984A1D: clone (in /lib/libc-2.4.so)
==20771== Address 0x4B4D0E2 is on thread 2's stack
Logged In: YES
user_id=541179
Are you certain that valgrind complained about *one* unitialized variable, not
*two*? The valgrind report does *not* indicate which line number in pcap-
linux.c made the setsockopt() call, perhaps because the call to p-
>setfilter_op in pcap_setfilter() was tail-call-optimized.
There are *multiple* setsockopt() calls in the setfilter code path.
There's a call with SO_ATTACH_FILTER and total_fcode as the filter argument.
total_fcode has static storage scope, so it would be uninitialized only if the C
compiler had a serious bug, so that's extremely unlikely to be the problematic
call.
Then there's the call with SO_ATTACH_FILTER and the fcode structure in
question. At least according to my copy of the Linux 2.6.17.3 source, "struct
sock_fprog" has two members, and both are set both by fix_program() if it
returns a value other than -1; the only code path where that call is made is
one where fix_program() returns 1 (if it returns -1, pcap_setfilter_linux()
returns before setting the filter; if it returns 0, it sets can_filter_in_kernel to 0
so pcap_setfilter_linux() doesn't try to set the kernel filter).
For that to cause an uninitialized reference, either:
1) the version of the kernel you're running would have to have added
another field to struct sock_fprog, in which case
1) they would have broken binary compatibility with all code using
SO_ATTACH_FILTER
and
2) without seeing their change, or a description of it, it cannot be
assumed that setting that field to 0 is the correct thing to do, so it cannot be
assumed that using memset will correctly fix the problem (i.e., using memset,
as I already explained, is not guaranteed to work, where "work" is defined as
"makes the code work correctly" rather than "makes valgrind not complain
about the code").
Then there's the call in reset_kernel_filter(); if there are errors setting the
kernel filter, that call is made to remove the "total filter". At least according
to the source code to valgrind 3.2.1, the check valgrind makes on the last two
arguments setsockopt() calls is that it checks that all N bytes pointed to by
the next-to-last argument are initialized, where N is the last argument; it
does not know, for example, that particular options, such as
SO_DETACH_FILTER, don't use their arguments. The dummy argument wasn't
initialized.
I've checked in a change to initialize the dummy argument to squelch the
valgrind complaint. If that's *not* the call that's causing the problem, then
either
1) an additional member has been added to the sock_fprog structure, in
which case we have to know what that member is used for before setting it
or
2) there's some code path where fix_program() isn't called, or returns -1,
but where the socket filter gets set, in which case *that's* the bug -
memsetting the structure merely papers over that bug.
Using memset here would be the equivalent of dealing with a cracked wall in
a building by sanding and re-painting the wall to cover up the crack, without
checking the structure of the building to see whether there are any serious
problems of which the crack is just a symptom.
Logged In: YES
user_id=1320060
In this case where the structure does not contain pointer,
your complain about memset is invalid.
The kernel can add whatever member to the struture, memset
will protect the code from using uninitialized variable.
The code has to run on any kernel, past, present or
future, without any error. If tomorow, kernel developpers
add a member, the code will be broken if it doesn't use
memset
By the way, memset is used 9 times in pcap_linux.c to
initialize structure, do you consider these as mistakes
too ?
What are the name of platform that doesn't initialized
pointer to 0 ?
Logged In: YES
user_id=541179
> In this case where the structure does not contain pointer,
your complain about memset is invalid.
> The kernel can add whatever member to the struture, memset
will protect the code from using uninitialized variable.
It will not, however, protect the code from using an *incorrectly initialized*
variable, if the correct value for the structure member isn't 0.
> The code has to run on any kernel, past, present or
future, without any error. If tomorow, kernel developpers
add a member, the code will be broken if it doesn't use
memset
As I have already explained, several times, the code could be broken even if it
*does* use memset, if, for example, the SO_ATTACH_FILTER API changes so
that you have to supply a pointer to some other item in order to get the
desired behavior, or if you have to supply a filter type code of some sort
where the type desired is *not* 0, or if you have to supply a set of flags and
one of the flags has to be set to get the desired behavior.
> By the way, memset is used 9 times in pcap_linux.c to
initialize structure, do you consider these as mistakes
too ?
If the structure isn't defined by libpcap, and if the structure is subject to
change, yes, I do.
> What are the name of platform that doesn't initialized
pointer to 0 ?
Define "initialize pointer to 0". The C programming language specification
says that a constant value of 0 is, when assigned to or compared with a
pointer value, converted to a "null pointer constant", which is not guaranteed
to have all its bits zero. Thus:
char *p;
p = 0;
and
char *p;
memset(&p, 0, sizeof p);
are *not* equivalent in standard C.
Here's an C FAQ discussing this:
http://c-faq.com/null/machexamp.html
Whether any C implementations have been done using those null pointer
representations is another matter, and it's probably quite unlikely that any
*interesting* platforms have that characteristic. I didn't add that as an
indication that it's worth worrying about whether memset(..., 0, ...) will
initialize pointers to 0 in libpcap, I just added it as a parenthetical note to
educate those who might think that C guarantees that all bits of a null pointer
are 0.
Logged In: YES
user_id=1320060
The solution is to use a contructor for the structure,
instead of using memset. Is there any contructor of the
fcode structure, I bet no. So a memset is the least worst
solution and acts a a shield to protect the application.
A value incorrectly initialized to 0 is definitly better
than an uninitialized value. At least a memset will not
make the application crashed
Using a constructor is better than memset, and memset is
better than individually initialized each member.
The 9 memset are almost all kernel structures.
When thinking about this problem, I'm so glad to have
moved to 00 programming, except of course when my app has
to link to a C library.
Administrators of the "libpcap" SourceForge project have superseded this tracker item (formerly artifact 1552777, now bug 75) with issue 77 of the "libpcap" GitHub project.