Quoting Dmitry V. Levin (2016-06-21 17:43:49)
> On Tue, Jun 21, 2016 at 02:42:46PM +0000, Fabien Siron wrote:
> [...]
> > +static unsigned long
> > +nlmsg_next(struct nlmsghdr *nlmsghdr, unsigned long addr, unsigned long
> > *len) {
> > + *len -= NLMSG_ALIGN(nlmsghdr->nlmsg_len);
> > +
> > + if (NLMSG_ALIGN(nlmsghdr->nlmsg_len) == 0 ||
> > + NLMSG_ALIGN(nlmsghdr->nlmsg_len) > *len)
>
> The check is too late.
It seems that the check has to be moved at the beginning of the function,
indeed.
>
> > + return 0;
> > +
> > + return (unsigned long)((addr) + NLMSG_ALIGN(nlmsghdr->nlmsg_len));
> > +}
>
> Why (addr)? Is this cast to (unsigned long) really needed?
> What's going to happen in case of integer overflow?
The following fix should be okay:
return addr + NLMSG_ALIGN(nlmsghdr->nlmsg_len) > addr
? addr + NLMSG_ALIGN(nlmsghdr->nlmsg_len) : 0;
>
> > +static void
> > +decode_netlink_msg(struct tcb *tcp, unsigned long addr,
> > + unsigned long size)
> > {
> > struct nlmsghdr nlmsghdr;
> >
> > @@ -57,8 +86,32 @@ decode_netlink(struct tcb *tcp, unsigned long addr,
> > unsigned long size)
> > if (size - sizeof(struct nlmsghdr) > 0) {
> > tprints(", ");
> > printstr(tcp, addr + sizeof(struct nlmsghdr),
> > - size - sizeof(struct nlmsghdr));
> > + nlmsghdr.nlmsg_len - sizeof(struct nlmsghdr));
> > }
> > tprints("}");
> > }
>
> What's going to be printed if size > sizeof(struct nlmsghdr)
> AND nlmsghdr.nlmsg_len == sizeof(struct nlmsghdr)?
I forgot to replace the first condition by:
if (nlmsghdr.nlmsg_len - sizeof(struct nlmsghdr))
>
> > +
> > +void
> > +decode_netlink(struct tcb *tcp, unsigned long addr, unsigned long
> > total_size) {
> > + struct nlmsghdr nlmsghdr;
> > + unsigned long elt, size = total_size;
> > +
> > + for (elt = 0; nlmsg_fetch(tcp, &nlmsghdr, addr, size);
> > + addr = nlmsg_next(&nlmsghdr, addr, &size), elt++) {
> > + if (elt == max_strlen && abbrev(tcp)) {
> > + tprints("...");
> > + break;
> > + }
> > + if (size != total_size)
> > + tprints(", ");
> > + else if (NLMSG_ALIGN(nlmsghdr.nlmsg_len) < total_size)
> > + tprints("[");
> > + decode_netlink_msg(tcp, addr, size);
> > + }
> > + if (nlmsghdr.nlmsg_len != (unsigned) -1 &&
> > + nlmsghdr.nlmsg_len != 0 &&
> > + NLMSG_ALIGN(nlmsghdr.nlmsg_len) < total_size) {
> > + tprints("]");
> > + }
> > +}
>
> This check doesn't look obvious; is it correct?
> Is there a more clear way to implement this?
>
If nlmsghdr.nlmsg_len is equal to -1, it means that umoven had failed.
If nlmsghdr.nlmsg_len is equal to 0, it means that nlmsg_fetch had failed.
I am not 100% sure of the last test but it seems that it's a correct way to
detect if there were several messages.
Have you got another idea to do this test?
Regards,
--
Fabien Siron
------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
_______________________________________________
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel
