Bug 315441 - sendmsg syscall should ignore unset msghdr msg_flags
Summary: sendmsg syscall should ignore unset msghdr msg_flags
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: memcheck (show other bugs)
Version: 3.9.0.SVN
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-19 10:34 UTC by Mark Wielaard
Modified: 2013-02-28 12:51 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
sendmsg-syscall-should-ignore-unset-msghd.patch (3.68 KB, patch)
2013-02-19 10:37 UTC, Mark Wielaard
Details
sendmsg-syscall-should-ignore-unset-msghd.patch (6.58 KB, patch)
2013-02-21 14:12 UTC, Mark Wielaard
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Wielaard 2013-02-19 10:34:01 UTC
msghdr msg_flags is ignored by the sendmsg syscall. It is the flags on the received message. But sendmsg doesn't recieve messages, it only sends them.

Example code snippet:

  // Create msg_hdr. Note that msg_flags isn't being set.
  msg.msg_name = NULL;
  msg.msg_namelen = 0;
  iov[0].iov_base = "one";
  iov[0].iov_len = 3;
  iov[1].iov_base = "two";
  iov[1].iov_len = 3;
  msg.msg_iov = &iov;
  msg.msg_iovlen = 2;
  msg.msg_control = NULL;
  msg.msg_controllen = 0;

  ssize_t s = sendmsg (fd, &msg, 0);

$ valgrind -q ./sendmsg
==2313== Syscall param socketcall.sendmsg(msg) points to uninitialised byte(s)
==2313==    at 0x34F0EE90A0: __sendmsg_nocancel (syscall-template.S:82)
==2313==    by 0x400771: main (sendmsg.c:46)
==2313==  Address 0x7ff000000 is on thread 1's stack
==2313== 

The same holds true for the new sendmmsg () syscall. Newer glibc (2.16) now use that systemcall (without setting the msg_flags field).

==8435== Syscall param sendmsg(mmsg[0].msg_hdr) points to uninitialised byte(s)
==8435==    at 0x34EFAF3D19: sendmmsg (sendmmsg.c:32)
==8435==    by 0x34F1A0B4D6: __libc_res_nsend (res_send.c:1132)
==8435==    by 0x34F1A08BDF: __libc_res_nquery (res_query.c:226)
==8435==    by 0x34F1A097B9: __libc_res_nsearch (res_query.c:582)
==8435==    by 0x13EDCAC7: _nss_dns_gethostbyname4_r (dns-host.c:309)


Reproducible: Always




Both the sendmsg and sendmmsg syscall use pre_mem_read_sendmsg for their checks (through 
generic_PRE_sys_sendmsg).

generic_PRE_sys_recvmsg () and pre_mem_write_recvmsg () does get this right:

static
void pre_mem_write_recvmsg ( ThreadId tid, Bool read,
                             const HChar *msg, Addr base, SizeT size )
{
   HChar *outmsg = strdupcat ( "di.syswrap.pmwr.1",
                               "recvmsg", msg, VG_AR_CORE );
   if ( read )
      PRE_MEM_READ( outmsg, base, size );
   else
      PRE_MEM_WRITE( outmsg, base, size );
   VG_(arena_free) ( VG_AR_CORE, outmsg );
}

But pre_mem_red_sendmsg does:

static
void pre_mem_read_sendmsg ( ThreadId tid, Bool read,
                            const HChar *msg, Addr base, SizeT size )
{
   HChar *outmsg = strdupcat ( "di.syswrap.pmrs.1",
                               "sendmsg", msg, VG_AR_CORE );
 
   PRE_MEM_READ( outmsg, base, size );
   VG_(arena_free) ( VG_AR_CORE, outmsg );
}

It should be:

static
void pre_mem_read_sendmsg ( ThreadId tid, Bool read,
                            const HChar *msg, Addr base, SizeT size )
{
   HChar *outmsg = strdupcat ( "di.syswrap.pmrs.1",
                               "sendmsg", msg, VG_AR_CORE );
   if ( read)
      PRE_MEM_READ( outmsg, base, size );
   VG_(arena_free) ( VG_AR_CORE, outmsg );
}
Comment 1 Mark Wielaard 2013-02-19 10:37:08 UTC
Created attachment 77422 [details]
sendmsg-syscall-should-ignore-unset-msghd.patch

coregrind/m_syswrap/syswrap-generic.c patch and memcheck/tests testcase
Comment 2 Tom Hughes 2013-02-21 11:07:08 UTC
That patch disables a whole load of checking that we do want though - it's not only flags that have "read" set to false but also things like the address in msg_name and the actual data in the iov.

The "read" argument tells you only one thing - whether that field is read or written (there is no option for ignored) and only for recvmsg. There is no equivalent for sendmsg.

We probably need to rename it and add an extra field so that we have separate read/written/ignored flags for recv and send.
Comment 3 Mark Wielaard 2013-02-21 14:12:15 UTC
Created attachment 77489 [details]
sendmsg-syscall-should-ignore-unset-msghd.patch

(In reply to comment #2)
> That patch disables a whole load of checking that we do want though - it's
> not only flags that have "read" set to false but also things like the
> address in msg_name and the actual data in the iov.
> 
> The "read" argument tells you only one thing - whether that field is read or
> written (there is no option for ignored) and only for recvmsg. There is no
> equivalent for sendmsg.

Doh. Sorry, missed that. How about this alternative patch? I extended the testcase to show a case (forgetting to set  msg_name) that was missed by my earlier patch,  but properly caught by this one.
Comment 4 Tom Hughes 2013-02-28 12:51:08 UTC
Committed as r13294.