|
From: Alan M. E. <val...@al...> - 2007-07-03 17:53:52
|
Hello.
Please consider the following function:
ssize_t nSendTo (int fd, const void *buf, size_t size, const char *host, port_t port) {
struct hostent *hp;
struct sockaddr_in to;
#ifdef VERROR
memset(&to,0,sizeof(to));
#endif
if (!(hp = gethostbyname(host)) &&
!(hp = gethostbyaddr(host,strlen(host),AF_INET))) return -1;
memcpy((char *)&to.sin_addr, hp->h_addr_list[0], hp->h_length);
to.sin_port = htons((short)port);
to.sin_family = AF_INET;
#ifdef VERROR
if (size==31) {
char buf31[100]= {0};
ssize_t sent;
for (sent = 0; sent<100; ++sent) buf31[sent] = 0;
printf("31 bytes\n");
memcpy(&buf31[0],buf,31);
sent = sendto(fd,&buf31[0],size,0,(struct sockaddr*)&to,sizeof(to));
printf("sent=%i\n",sent);
return sent;
} else {
printf("%i bytes\n",size);
#endif
return sendto(fd,buf,size,0,(struct sockaddr*)&to,sizeof(to));
#ifdef VERROR
}
#endif
}
Without the VERROR sections, this function has served me well for years
as a wrapper around sendto(). Now, valgrind is complaining about
uninitialized bytes, but only the first time size==31:
22 bytes
12 bytes
15 bytes
54 bytes
12 bytes
22 bytes
12 bytes
15 bytes
33 bytes
31 bytes
==545== Syscall param socketcall.sendto(msg) points to uninitialised byte(s)
==545== at 0x34082CE283: __sendto_nocancel (in /lib64/libc-2.5.so)
==545== by 0x40899C: nSendTo (in /home/alan/bin/Neti)
==545== by 0x409745: nWriteTo (in /home/alan/bin/Neti)
==545== by 0x40BFA6: NetiSession::HandleWrite() (in /home/alan/bin/Neti)
==545== by 0x4113C7: Session::Check(SelectSet*) (in /home/alan/bin/Neti)
==545== by 0x40258E: CheckSession(Monad*, void*) (in /home/alan/bin/Neti)
==545== by 0x407D04: MonadList::ForEach(bool (*)(Monad*, void*), void*) (in /home/alan/bin/Neti)
==545== by 0x402CE0: Run(int, char**, char**, char*) (in /home/alan/bin/Neti)
==545== by 0x402DFC: main (in /home/alan/bin/Neti)
==545== Address 0x7FF00015C is on thread 1's stack
sent=31
22 bytes
I added the VERROR sections to be supremely sure that I could not
possibly be passing any uninitialized bytes to sendto(). Did I miss
something? Is valgrind misreporting? Or is glibc doing the wrong thing?
How would I find out?
For information, my development platform is FC6.x86_64 on AMD Athlon X2.
Valgrind is version 3.2.3 from the development repo.
-Alan
|
|
From: Tom H. <to...@co...> - 2007-07-03 18:02:55
|
In message <118...@sk...>
"Alan M. Evans" <val...@al...> wrote:
> Please consider the following function:
>
> ssize_t nSendTo (int fd, const void *buf, size_t size, const char *host, port_t port) {
> struct hostent *hp;
> struct sockaddr_in to;
> #ifdef VERROR
> memset(&to,0,sizeof(to));
> #endif
> if (!(hp = gethostbyname(host)) &&
> !(hp = gethostbyaddr(host,strlen(host),AF_INET))) return -1;
> memcpy((char *)&to.sin_addr, hp->h_addr_list[0], hp->h_length);
> to.sin_port = htons((short)port);
> to.sin_family = AF_INET;
> #ifdef VERROR
> if (size==31) {
> char buf31[100]= {0};
> ssize_t sent;
> for (sent = 0; sent<100; ++sent) buf31[sent] = 0;
> printf("31 bytes\n");
> memcpy(&buf31[0],buf,31);
> sent = sendto(fd,&buf31[0],size,0,(struct sockaddr*)&to,sizeof(to));
> printf("sent=%i\n",sent);
> return sent;
> } else {
> printf("%i bytes\n",size);
> #endif
> return sendto(fd,buf,size,0,(struct sockaddr*)&to,sizeof(to));
> #ifdef VERROR
> }
> #endif
> }
>
> Without the VERROR sections, this function has served me well for years
> as a wrapper around sendto(). Now, valgrind is complaining about
> uninitialized bytes, but only the first time size==31:
>
> 22 bytes
> 12 bytes
> 15 bytes
> 54 bytes
> 12 bytes
> 22 bytes
> 12 bytes
> 15 bytes
> 33 bytes
> 31 bytes
> ==545== Syscall param socketcall.sendto(msg) points to uninitialised byte(s)
> ==545== at 0x34082CE283: __sendto_nocancel (in /lib64/libc-2.5.so)
> ==545== by 0x40899C: nSendTo (in /home/alan/bin/Neti)
> ==545== by 0x409745: nWriteTo (in /home/alan/bin/Neti)
> ==545== by 0x40BFA6: NetiSession::HandleWrite() (in /home/alan/bin/Neti)
> ==545== by 0x4113C7: Session::Check(SelectSet*) (in /home/alan/bin/Neti)
> ==545== by 0x40258E: CheckSession(Monad*, void*) (in /home/alan/bin/Neti)
> ==545== by 0x407D04: MonadList::ForEach(bool (*)(Monad*, void*), void*) (in /home/alan/bin/Neti)
> ==545== by 0x402CE0: Run(int, char**, char**, char*) (in /home/alan/bin/Neti)
> ==545== by 0x402DFC: main (in /home/alan/bin/Neti)
> ==545== Address 0x7FF00015C is on thread 1's stack
> sent=31
> 22 bytes
>
> I added the VERROR sections to be supremely sure that I could not
> possibly be passing any uninitialized bytes to sendto(). Did I miss
> something? Is valgrind misreporting? Or is glibc doing the wrong thing?
> How would I find out?
I don't understand what you think the VERROR sections are proving?
You carefuly zero fill 100 bytes, but then you copy in the 31 bytes
that you are actually going to send, and which presumably contain
the uninitialised data, hence undoing all the time you spent carefully
zero filling the local array.
The real problem presumably lies somewhere further up the call stack
where some uninitialised data was placed in the buffer that it is
being asked to send.
Tom
--
Tom Hughes (to...@co...)
http://www.compton.nu/
|
|
From: Alan M. E. <val...@al...> - 2007-07-05 22:48:34
|
On Tue, 2007-07-03 at 19:02 +0100, Tom Hughes wrote:
> In message <118...@sk...>
> "Alan M. Evans" <val...@al...> wrote:
>
> > Please consider the following function:
> >
> > ssize_t nSendTo (int fd, const void *buf, size_t size, const char *host, port_t port) {
> > struct hostent *hp;
> > struct sockaddr_in to;
> > #ifdef VERROR
> > memset(&to,0,sizeof(to));
> > #endif
> > if (!(hp = gethostbyname(host)) &&
> > !(hp = gethostbyaddr(host,strlen(host),AF_INET))) return -1;
> > memcpy((char *)&to.sin_addr, hp->h_addr_list[0], hp->h_length);
> > to.sin_port = htons((short)port);
> > to.sin_family = AF_INET;
> > #ifdef VERROR
> > if (size==31) {
> > char buf31[100]= {0};
> > ssize_t sent;
> > for (sent = 0; sent<100; ++sent) buf31[sent] = 0;
> > printf("31 bytes\n");
> > memcpy(&buf31[0],buf,31);
> > sent = sendto(fd,&buf31[0],size,0,(struct sockaddr*)&to,sizeof(to));
> > printf("sent=%i\n",sent);
> > return sent;
> > } else {
> > printf("%i bytes\n",size);
> > #endif
> > return sendto(fd,buf,size,0,(struct sockaddr*)&to,sizeof(to));
> > #ifdef VERROR
> > }
> > #endif
> > }
> >
> > Without the VERROR sections, this function has served me well for years
> > as a wrapper around sendto(). Now, valgrind is complaining about
> > uninitialized bytes, but only the first time size==31:
> >
> > 22 bytes
> > 12 bytes
> > 15 bytes
> > 54 bytes
> > 12 bytes
> > 22 bytes
> > 12 bytes
> > 15 bytes
> > 33 bytes
> > 31 bytes
> > ==545== Syscall param socketcall.sendto(msg) points to uninitialised byte(s)
> > ==545== at 0x34082CE283: __sendto_nocancel (in /lib64/libc-2.5.so)
> > ==545== by 0x40899C: nSendTo (in /home/alan/bin/Neti)
> > ==545== by 0x409745: nWriteTo (in /home/alan/bin/Neti)
> > ==545== by 0x40BFA6: NetiSession::HandleWrite() (in /home/alan/bin/Neti)
> > ==545== by 0x4113C7: Session::Check(SelectSet*) (in /home/alan/bin/Neti)
> > ==545== by 0x40258E: CheckSession(Monad*, void*) (in /home/alan/bin/Neti)
> > ==545== by 0x407D04: MonadList::ForEach(bool (*)(Monad*, void*), void*) (in /home/alan/bin/Neti)
> > ==545== by 0x402CE0: Run(int, char**, char**, char*) (in /home/alan/bin/Neti)
> > ==545== by 0x402DFC: main (in /home/alan/bin/Neti)
> > ==545== Address 0x7FF00015C is on thread 1's stack
> > sent=31
> > 22 bytes
> >
> > I added the VERROR sections to be supremely sure that I could not
> > possibly be passing any uninitialized bytes to sendto(). Did I miss
> > something? Is valgrind misreporting? Or is glibc doing the wrong thing?
> > How would I find out?
>
> I don't understand what you think the VERROR sections are proving?
>
> You carefuly zero fill 100 bytes, but then you copy in the 31 bytes
> that you are actually going to send, and which presumably contain
> the uninitialised data, hence undoing all the time you spent carefully
> zero filling the local array.
Fair enough. This is where I ended. I had, of course, started without
the VERROR bits, and assumed that the uninitialized bytes were further
up the stack. The overall project is very large, well over 10000 lines,
and I spent the better part of an entire day meticulously hand debugging
to prove that every byte was, indeed, being assigned something.
When I finally got to this relatively simple example, I figured, "Well
how can *that* be?" What I missed here, apparently, is that memcpy can
copy uninitialized bytes and valgrind won't say so at that point. I
assume that there's a good reason for that, probably something to do
with struct padding.
> The real problem presumably lies somewhere further up the call stack
> where some uninitialised data was placed in the buffer that it is
> being asked to send.
Ok then, how about this?:
bool NetiSession::HandleWrite() {
int len;
Buffer *buffer = static_cast<Buffer*>(buffers.ForEach(FindFirst,0));
if (buffer) { // This should *always* be true
#ifdef VERROR
{
char *b = (char*)buffer->Buf();
g_print("copying %i bytes from %lx prior to sendto\n",buffer->Len(),(unsigned long)buffer->Buf());
for (len=0;len<buffer->Len();++len) { global_debug_buffer[len] = b[len]; }
}
#endif
len = nWriteTo(fd,buffer->Buf(),buffer->Len(),context->remote_host,context->remote_port);
buffers.Remove(buffer);
}
return false;
}
Here, I am checking all the bytes in the buffer by copying them
(one-by-one, no memcpy) into a global char array. Compiler optimization
is off and the destination array is global, so the line should not be
optimized away. This is not triggering any warnings from valgrind:
copying 31 bytes from 4ee7e30 prior to sendto
==4438== Syscall param socketcall.sendto(msg) points to uninitialised byte(s)
==4438== at 0x34082CE283: __sendto_nocancel (in /lib64/libc-2.5.so)
==4438== by 0x4C20094: nSendTo (nci.c:401)
==4438== by 0x4C2028D: nWriteTo (nci.c:471)
==4438== by 0x40B29F: NetiSession::HandleWrite() (netisession.cpp:584)
==4438== by 0x40FF10: Session::Check(SelectSet*) (session.cpp:380)
==4438== by 0x4022DC: CheckSession(Monad*, void*) (Neti.cpp:39)
==4438== by 0x409BE6: MonadList::ForEach(bool (*)(Monad*, void*), void*) (monad.cpp:148)
==4438== by 0x402E56: Run(int, char**, char**, char*) (Neti.cpp:170)
==4438== by 0x403108: main (Neti.cpp:398)
==4438== Address 0x4EE7E4C is 52 bytes inside a block of size 1,584 alloc'd
==4438== at 0x4A061A5: operator new(unsigned long) (vg_replace_malloc.c:167)
==4438== by 0x40D239: NetiSession::Prepare(SelectSet*) (netisession.cpp:131)
==4438== by 0x40229E: PrepareSession(Monad*, void*) (Neti.cpp:31)
==4438== by 0x409BE6: MonadList::ForEach(bool (*)(Monad*, void*), void*) (monad.cpp:148)
==4438== by 0x402E1E: Run(int, char**, char**, char*) (Neti.cpp:165)
==4438== by 0x403108: main (Neti.cpp:398)
So the eventual call to sendto() with this buffer is triggering an
uninitialized warning. The "bad" address (0x4EE7E4C) is 28 bytes into my
verified 31 byte buffer at 0x4ee7e30.
I'm suspecting that the library sendto is reading the buffer a word at a
time and so overrunning my buffer. Odd that it only does it when the
buffer is exactly 31 bytes and only the first time.
-Alan
|
|
From: Tom H. <to...@co...> - 2007-07-05 23:23:16
|
In message <118...@sk...>
"Alan M. Evans" <val...@al...> wrote:
> On Tue, 2007-07-03 at 19:02 +0100, Tom Hughes wrote:
>
> > You carefuly zero fill 100 bytes, but then you copy in the 31 bytes
> > that you are actually going to send, and which presumably contain
> > the uninitialised data, hence undoing all the time you spent carefully
> > zero filling the local array.
>
> Fair enough. This is where I ended. I had, of course, started without
> the VERROR bits, and assumed that the uninitialized bytes were further
> up the stack. The overall project is very large, well over 10000 lines,
> and I spent the better part of an entire day meticulously hand debugging
> to prove that every byte was, indeed, being assigned something.
>
> When I finally got to this relatively simple example, I figured, "Well
> how can *that* be?" What I missed here, apparently, is that memcpy can
> copy uninitialized bytes and valgrind won't say so at that point. I
> assume that there's a good reason for that, probably something to do
> with struct padding.
Copy of unintialised data is not reported - see the manual:
http://www.valgrind.org/docs/manual/mc-manual.html#mc-manual.uninitvals
The reason is basically that it produces far too many false positives
because in turns out that programs do it all the time quite legitimately.
Struct padding is indeed one source of such things.
So valgrind only complains about an uninitialised value when it could
affect the behaviour of the program - when it is used in conditional
execution, when you try and read or write through an uninitialised
pointer, when you pass uninitialised data to a system call, etc.
> > The real problem presumably lies somewhere further up the call stack
> > where some uninitialised data was placed in the buffer that it is
> > being asked to send.
>
> Ok then, how about this?:
>
> bool NetiSession::HandleWrite() {
> int len;
> Buffer *buffer = static_cast<Buffer*>(buffers.ForEach(FindFirst,0));
> if (buffer) { // This should *always* be true
> #ifdef VERROR
> {
> char *b = (char*)buffer->Buf();
> g_print("copying %i bytes from %lx prior to sendto\n",buffer->Len(),(unsigned long)buffer->Buf());
> for (len=0;len<buffer->Len();++len) { global_debug_buffer[len] = b[len]; }
> }
> #endif
> len = nWriteTo(fd,buffer->Buf(),buffer->Len(),context->remote_host,context->remote_port);
> buffers.Remove(buffer);
> }
> return false;
> }
>
> Here, I am checking all the bytes in the buffer by copying them
> (one-by-one, no memcpy) into a global char array. Compiler optimization
> is off and the destination array is global, so the line should not be
> optimized away. This is not triggering any warnings from valgrind:
Still the same though - copying byte by byte still won't trigger any
reports by valgrind.
> copying 31 bytes from 4ee7e30 prior to sendto
> ==4438== Syscall param socketcall.sendto(msg) points to uninitialised byte(s)
> ==4438== at 0x34082CE283: __sendto_nocancel (in /lib64/libc-2.5.so)
> ==4438== by 0x4C20094: nSendTo (nci.c:401)
> ==4438== by 0x4C2028D: nWriteTo (nci.c:471)
> ==4438== by 0x40B29F: NetiSession::HandleWrite() (netisession.cpp:584)
> ==4438== by 0x40FF10: Session::Check(SelectSet*) (session.cpp:380)
> ==4438== by 0x4022DC: CheckSession(Monad*, void*) (Neti.cpp:39)
> ==4438== by 0x409BE6: MonadList::ForEach(bool (*)(Monad*, void*), void*) (monad.cpp:148)
> ==4438== by 0x402E56: Run(int, char**, char**, char*) (Neti.cpp:170)
> ==4438== by 0x403108: main (Neti.cpp:398)
> ==4438== Address 0x4EE7E4C is 52 bytes inside a block of size 1,584 alloc'd
> ==4438== at 0x4A061A5: operator new(unsigned long) (vg_replace_malloc.c:167)
> ==4438== by 0x40D239: NetiSession::Prepare(SelectSet*) (netisession.cpp:131)
> ==4438== by 0x40229E: PrepareSession(Monad*, void*) (Neti.cpp:31)
> ==4438== by 0x409BE6: MonadList::ForEach(bool (*)(Monad*, void*), void*) (monad.cpp:148)
> ==4438== by 0x402E1E: Run(int, char**, char**, char*) (Neti.cpp:165)
> ==4438== by 0x403108: main (Neti.cpp:398)
>
> So the eventual call to sendto() with this buffer is triggering an
> uninitialized warning. The "bad" address (0x4EE7E4C) is 28 bytes into my
> verified 31 byte buffer at 0x4ee7e30.
>
> I'm suspecting that the library sendto is reading the buffer a word at a
> time and so overrunning my buffer. Odd that it only does it when the
> buffer is exactly 31 bytes and only the first time.
It's not any read the library is doing that is complaining - it is
the wrapper for the system call itself and that is precise.
It's quite simple - the byte at that offset in the buffer passed to
your routine is uninitialised.
Tom
--
Tom Hughes (to...@co...)
http://www.compton.nu/
|
|
From: Christoph B. <bar...@or...> - 2007-07-05 22:59:51
|
Am Donnerstag, 5. Juli 2007 schrieb Alan M. Evans:
>
> bool NetiSession::HandleWrite() {
> int len;
> Buffer *buffer =
> static_cast<Buffer*>(buffers.ForEach(FindFirst,0)); if (buffer) { // This
> should *always* be true
> #ifdef VERROR
> {
> char *b = (char*)buffer->Buf();
> g_print("copying %i bytes from %lx prior to
> sendto\n",buffer->Len(),(unsigned long)buffer->Buf()); for
> (len=0;len<buffer->Len();++len) { global_debug_buffer[len] = b[len]; } }
> #endif
> len =
> nWriteTo(fd,buffer->Buf(),buffer->Len(),context->remote_host,context->remot
>e_port); buffers.Remove(buffer);
> }
> return false;
> }
>
> Here, I am checking all the bytes in the buffer by copying them
> (one-by-one, no memcpy) into a global char array. Compiler optimization
> is off and the destination array is global, so the line should not be
> optimized away. This is not triggering any warnings from valgrind:
Copying uninitializied bytes does not trigger a warning from valgrind, because
there is no decision made on the bytes. If there is no decision on a byte, it
has no effect on the program flow.
If you want to know which byte is not initialized use the following code:
#include <valgrind/memcheck.h>
...
for (len=0; len < buffer->Len(); ++len) {
VALGRIND_CHECK_VALUE_IS_DEFINED(b[len]);
}
...
Christoph
|
|
From: Alan M. E. <val...@al...> - 2007-07-06 00:01:20
|
On Fri, 2007-07-06 at 00:59 +0200, Christoph Bartoschek wrote:
> Copying uninitializied bytes does not trigger a warning from valgrind, because
> there is no decision made on the bytes. If there is no decision on a byte, it
> has no effect on the program flow.
Wow. Misunderstanding this has lead me to so many wrong conclusions.
> If you want to know which byte is not initialized use the following code:
>
> #include <valgrind/memcheck.h>
>
> ...
> for (len=0; len < buffer->Len(); ++len) {
> VALGRIND_CHECK_VALUE_IS_DEFINED(b[len]);
> }
> ...
I suppose I should have guessed that a facility such as this was
available. Very useful.
Using this, I was able to trace this back to a constructor that was
doing:
this->foo = foo;
Copied from another constructor that actually had foo as a parameter,
but in this one, it just worked out to a nop.
If I had not assumed that valgrind would tell me when I read foo (copy
to buffer), I would have had this licked a long time ago. Knowledge is
power.
Thanks to you and also Tom Hughes for putting up with my dimwitted
query. Even experienced programmers just sometimes can't see past their
assumptions.
-Alan
|