|
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
|