ping6(8) bug

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

ping6(8) bug

Michael McConville-2
It seems pretty clear to me that what was here was wrong. A field of a
global struct was pointed at local array. The program logic was a little
wacky, but this is my best estimate of what was intended. Input?


Index: ping6.c
===================================================================
RCS file: /cvs/src/sbin/ping6/ping6.c,v
retrieving revision 1.112
diff -u -p -r1.112 ping6.c
--- ping6.c 1 Sep 2015 19:53:23 -0000 1.112
+++ ping6.c 7 Sep 2015 19:44:15 -0000
@@ -1056,7 +1056,10 @@ pinger(void)
  memset(&iov, 0, sizeof(iov));
  iov[0].iov_base = (caddr_t)outpack;
  iov[0].iov_len = cc;
- smsghdr.msg_iov = iov;
+ smsghdr.msg_iov = calloc(1, sizeof(struct iovec));
+ if (smsghdr.msg_iov == NULL)
+ return(1);
+ *smsghdr.msg_iov = iov[0];
  smsghdr.msg_iovlen = 1;
 
  i = sendmsg(s, &smsghdr, 0);

Reply | Threaded
Open this post in threaded view
|

Re: ping6(8) bug

Michael McConville-2
We'd have to determine where and when this memory gets freed, of course.

Michael McConville wrote:

> It seems pretty clear to me that what was here was wrong. A field of a
> global struct was pointed at local array. The program logic was a
> little wacky, but this is my best estimate of what was intended.
> Input?
>
>
> Index: ping6.c
> ===================================================================
> RCS file: /cvs/src/sbin/ping6/ping6.c,v
> retrieving revision 1.112
> diff -u -p -r1.112 ping6.c
> --- ping6.c 1 Sep 2015 19:53:23 -0000 1.112
> +++ ping6.c 7 Sep 2015 19:44:15 -0000
> @@ -1056,7 +1056,10 @@ pinger(void)
>   memset(&iov, 0, sizeof(iov));
>   iov[0].iov_base = (caddr_t)outpack;
>   iov[0].iov_len = cc;
> - smsghdr.msg_iov = iov;
> + smsghdr.msg_iov = calloc(1, sizeof(struct iovec));
> + if (smsghdr.msg_iov == NULL)
> + return(1);
> + *smsghdr.msg_iov = iov[0];
>   smsghdr.msg_iovlen = 1;
>  
>   i = sendmsg(s, &smsghdr, 0);

Reply | Threaded
Open this post in threaded view
|

Re: ping6(8) bug

Claudio Jeker
In reply to this post by Michael McConville-2
On Mon, Sep 07, 2015 at 03:49:14PM -0400, Michael McConville wrote:

> It seems pretty clear to me that what was here was wrong. A field of a
> global struct was pointed at local array. The program logic was a little
> wacky, but this is my best estimate of what was intended. Input?
>
>
> Index: ping6.c
> ===================================================================
> RCS file: /cvs/src/sbin/ping6/ping6.c,v
> retrieving revision 1.112
> diff -u -p -r1.112 ping6.c
> --- ping6.c 1 Sep 2015 19:53:23 -0000 1.112
> +++ ping6.c 7 Sep 2015 19:44:15 -0000
> @@ -1056,7 +1056,10 @@ pinger(void)
>   memset(&iov, 0, sizeof(iov));
>   iov[0].iov_base = (caddr_t)outpack;
>   iov[0].iov_len = cc;
> - smsghdr.msg_iov = iov;
> + smsghdr.msg_iov = calloc(1, sizeof(struct iovec));
> + if (smsghdr.msg_iov == NULL)
> + return(1);
> + *smsghdr.msg_iov = iov[0];
>   smsghdr.msg_iovlen = 1;
>  
>   i = sendmsg(s, &smsghdr, 0);
>

The current code is perfectly fine. msg_iov is a pointer and it points to
the iov array. When calling sendmsg the kernel will know how to handle
this pointer. There is no need to calloc() memory for this.

--
:wq Claudio

Reply | Threaded
Open this post in threaded view
|

Re: ping6(8) bug

Michael McConville-2
Claudio Jeker wrote:

> On Mon, Sep 07, 2015 at 03:49:14PM -0400, Michael McConville wrote:
> > It seems pretty clear to me that what was here was wrong. A field of
> > a global struct was pointed at local array. The program logic was a
> > little wacky, but this is my best estimate of what was intended.
> > Input?
> >
> >
> > Index: ping6.c
> > ===================================================================
> > RCS file: /cvs/src/sbin/ping6/ping6.c,v
> > retrieving revision 1.112
> > diff -u -p -r1.112 ping6.c
> > --- ping6.c 1 Sep 2015 19:53:23 -0000 1.112
> > +++ ping6.c 7 Sep 2015 19:44:15 -0000
> > @@ -1056,7 +1056,10 @@ pinger(void)
> >   memset(&iov, 0, sizeof(iov));
> >   iov[0].iov_base = (caddr_t)outpack;
> >   iov[0].iov_len = cc;
> > - smsghdr.msg_iov = iov;
> > + smsghdr.msg_iov = calloc(1, sizeof(struct iovec));
> > + if (smsghdr.msg_iov == NULL)
> > + return(1);
> > + *smsghdr.msg_iov = iov[0];
> >   smsghdr.msg_iovlen = 1;
> >  
> >   i = sendmsg(s, &smsghdr, 0);
> >
>
> The current code is perfectly fine. msg_iov is a pointer and it points
> to the iov array. When calling sendmsg the kernel will know how to
> handle this pointer. There is no need to calloc() memory for this.

Yes, but the data that it points to is an array on pinger()'s stack.
When the current pinger() call returns, that data is gone and
smsghdr.msg_iov points to garbage. Remember, smsghdr is global.

That said, if we're sure that smsghdr.msg_iov is only accessed from
within the current pinger() call, this isn't a functional bug. It could
become one in the future, though, if someone adds something.

On a separate note, I just noticed that my use of calloc() here is a
little awkward. I was initially allocating space for two structs. It
could be malloc() instead.

Anyway, the more I look at this, the more I think it should just be
left as is, despite being a little precarious.

Reply | Threaded
Open this post in threaded view
|

Re: ping6(8) bug

Claudio Jeker
On Tue, Sep 08, 2015 at 02:31:32AM -0400, Michael McConville wrote:

> Claudio Jeker wrote:
> > On Mon, Sep 07, 2015 at 03:49:14PM -0400, Michael McConville wrote:
> > > It seems pretty clear to me that what was here was wrong. A field of
> > > a global struct was pointed at local array. The program logic was a
> > > little wacky, but this is my best estimate of what was intended.
> > > Input?
> > >
> > >
> > > Index: ping6.c
> > > ===================================================================
> > > RCS file: /cvs/src/sbin/ping6/ping6.c,v
> > > retrieving revision 1.112
> > > diff -u -p -r1.112 ping6.c
> > > --- ping6.c 1 Sep 2015 19:53:23 -0000 1.112
> > > +++ ping6.c 7 Sep 2015 19:44:15 -0000
> > > @@ -1056,7 +1056,10 @@ pinger(void)
> > >   memset(&iov, 0, sizeof(iov));
> > >   iov[0].iov_base = (caddr_t)outpack;
> > >   iov[0].iov_len = cc;
> > > - smsghdr.msg_iov = iov;
> > > + smsghdr.msg_iov = calloc(1, sizeof(struct iovec));
> > > + if (smsghdr.msg_iov == NULL)
> > > + return(1);
> > > + *smsghdr.msg_iov = iov[0];
> > >   smsghdr.msg_iovlen = 1;
> > >  
> > >   i = sendmsg(s, &smsghdr, 0);
> > >
> >
> > The current code is perfectly fine. msg_iov is a pointer and it points
> > to the iov array. When calling sendmsg the kernel will know how to
> > handle this pointer. There is no need to calloc() memory for this.
>
> Yes, but the data that it points to is an array on pinger()'s stack.
> When the current pinger() call returns, that data is gone and
> smsghdr.msg_iov points to garbage. Remember, smsghdr is global.
>
> That said, if we're sure that smsghdr.msg_iov is only accessed from
> within the current pinger() call, this isn't a functional bug. It could
> become one in the future, though, if someone adds something.
>
> On a separate note, I just noticed that my use of calloc() here is a
> little awkward. I was initially allocating space for two structs. It
> could be malloc() instead.
>
> Anyway, the more I look at this, the more I think it should just be
> left as is, despite being a little precarious.

Yeah, it is not optimal even though the smsghdr is only used in one place.
Anyway how about this diff instead?
Cleanup some of the mess and use the global iov struct which is currently
unused.
--
:wq Claudio

Index: ping6.c
===================================================================
RCS file: /cvs/src/sbin/ping6/ping6.c,v
retrieving revision 1.112
diff -u -p -r1.112 ping6.c
--- ping6.c 1 Sep 2015 19:53:23 -0000 1.112
+++ ping6.c 8 Sep 2015 16:27:43 -0000
@@ -208,7 +208,7 @@ u_short naflags;
 /* for ancillary data(advanced API) */
 struct msghdr smsghdr;
 struct iovec smsgiov;
-char *scmsg = 0;
+char *scmsg;
 
 volatile sig_atomic_t seenalrm;
 volatile sig_atomic_t seenint;
@@ -273,10 +273,6 @@ main(int argc, char *argv[])
  if (setresuid(uid, uid, uid) == -1)
  err(1, "setresuid");
 
- /* just to be sure */
- memset(&smsghdr, 0, sizeof(smsghdr));
- memset(&smsgiov, 0, sizeof(smsgiov));
-
  preload = 0;
  datap = &outpack[ICMP6ECHOLEN + ICMP6ECHOTMLEN];
  while ((ch = getopt(argc, argv,
@@ -787,7 +783,7 @@ main(int argc, char *argv[])
  struct cmsghdr hdr;
  u_char buf[CMSG_SPACE(1024)];
  } cmsgbuf;
- struct iovec iov[2];
+ struct iovec iov[1];
  struct pollfd pfd;
  ssize_t cc;
  int timeout;
@@ -952,7 +948,6 @@ int
 pinger(void)
 {
  struct icmp6_hdr *icp;
- struct iovec iov[2];
  int i, cc;
  struct icmp6_nodeinfo *nip;
  int seq;
@@ -1053,10 +1048,9 @@ pinger(void)
 
  smsghdr.msg_name = &dst;
  smsghdr.msg_namelen = sizeof(dst);
- memset(&iov, 0, sizeof(iov));
- iov[0].iov_base = (caddr_t)outpack;
- iov[0].iov_len = cc;
- smsghdr.msg_iov = iov;
+ smsgiov.iov_base = (caddr_t)outpack;
+ smsgiov.iov_len = cc;
+ smsghdr.msg_iov = &smsgiov;
  smsghdr.msg_iovlen = 1;
 
  i = sendmsg(s, &smsghdr, 0);