| Message ID | 20150128112734.56a18dda@redhat.com |
|---|---|
| State | New |
| Headers | show |
Siddhesh Poyarekar <siddhesh@redhat.com> writes: > nscd stats data that is sent from the daemon to the binary invoked with > 'nscd -g' does not always initialize selinux data. Valgrind complains > about it, so just initialize the entire struct once to get rid of that > warning. I haven't done a full analysis of the code changed, but the > net change in the code size of the send_stats function is just 2 > bytes. The valgrind warning: I don't think this has anything to do with selinux data, it's just complaining about uninitialised padding bytes, doesn't it? Andreas.
On Wed, 28 Jan 2015 10:18:53 +0100 Andreas Schwab <schwab@suse.de> wrote: > Siddhesh Poyarekar <siddhesh@redhat.com> writes: > > > nscd stats data that is sent from the daemon to the binary invoked > > with 'nscd -g' does not always initialize selinux data. Valgrind > > complains about it, so just initialize the entire struct once to > > get rid of that warning. I haven't done a full analysis of the > > code changed, but the net change in the code size of the send_stats > > function is just 2 bytes. The valgrind warning: > > I don't think this has anything to do with selinux data, it's just > complaining about uninitialised padding bytes, doesn't it? Ahh yes, it's not the selinux data, it is the padding bytes in the struct. So initializing the entire struct would in fact be the best way to fix this warning. Siddhesh
It always seems preferable to use an initializer.
On 01/28/2015 12:57 AM, Siddhesh Poyarekar wrote: > Hi, > > nscd stats data that is sent from the daemon to the binary invoked with > 'nscd -g' does not always initialize selinux data. Valgrind complains > about it, so just initialize the entire struct once to get rid of that > warning. I haven't done a full analysis of the code changed, but the > net change in the code size of the send_stats function is just 2 > bytes. The valgrind warning: > > #: valgrind nscd -d > ==11384== Memcheck, a memory error detector > ==11384== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al. > ==11384== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info > ==11384== Command: nscd -d > ==11384== > Fri 25 Apr 2014 10:34:53 AM CEST - 11384: handle_request: request received (Version = 2) from PID 11396 > Fri 25 Apr 2014 10:34:53 AM CEST - 11384: GETSTAT > ==11384== Thread 6: > ==11384== Syscall param socketcall.sendto(msg) points to uninitialised byte(s) > ==11384== at 0x4E4ACDC: send (in /lib64/libpthread-2.12.so) > ==11384== by 0x11AF6B: send_stats (in /usr/sbin/nscd) > ==11384== by 0x112F75: nscd_run_worker (in /usr/sbin/nscd) > ==11384== by 0x4E439D0: start_thread (in /lib64/libpthread-2.12.so) > ==11384== by 0x599AB6C: clone (in /lib64/libc-2.12.so) > ==11384== Address 0x15708395 is on thread 6's stack > > OK to commit to 2.21? Yes. Please commit this immediately. We should be zeroing stack structures before using them to send data to remote systems. While nscd won't leak directly important information, it's leaking some information that might some day be used indirectly. The cost of the memset is also miniscule compared to the cost of the sendto() and thus I'm not worried about the performance of this function. I'm more concerned about having a clean valgrind run and thus avoid confusing users when they see the above. Cheers, Carlos. > Siddhesh > > [BZ #17892] > * nscd/nscd_stat.c (send_stats): Initialize DATA. > > diff --git a/nscd/nscd_stat.c b/nscd/nscd_stat.c > index 0f1f3c0..7aaa21b 100644 > --- a/nscd/nscd_stat.c > +++ b/nscd/nscd_stat.c > @@ -94,6 +94,8 @@ send_stats (int fd, struct database_dyn dbs[lastdb]) > struct statdata data; > int cnt; > > + memset (&data, 0, sizeof (data)); > + > memcpy (data.version, compilation, sizeof (compilation)); > data.debug_level = debug_level; > data.runtime = time (NULL) - start_time; >
diff --git a/nscd/nscd_stat.c b/nscd/nscd_stat.c index 0f1f3c0..7aaa21b 100644 --- a/nscd/nscd_stat.c +++ b/nscd/nscd_stat.c @@ -94,6 +94,8 @@ send_stats (int fd, struct database_dyn dbs[lastdb]) struct statdata data; int cnt; + memset (&data, 0, sizeof (data)); + memcpy (data.version, compilation, sizeof (compilation)); data.debug_level = debug_level; data.runtime = time (NULL) - start_time;