Quoting Dmitry V. Levin (2016-07-04 22:14:40)
> On Wed, Jun 29, 2016 at 12:20:00PM +0000, Fabien Siron wrote:
> > +static unsigned long
> > +next_nlmsg(struct nlmsghdr *nlmsghdr, unsigned long addr, unsigned long
> > *len) {
> > + if (NLMSG_ALIGN(nlmsghdr->nlmsg_len) == 0 ||
> > + NLMSG_ALIGN(nlmsghdr->nlmsg_len) > *len) {
> > + *len = NLMSG_ALIGN(nlmsghdr->nlmsg_len);
> > + return 0;
> > + }
>
> *len may increase as result
>
> > + *len -= NLMSG_ALIGN(nlmsghdr->nlmsg_len);
> > +
> > + if (addr + NLMSG_ALIGN(nlmsghdr->nlmsg_len) > addr) {
> > + return addr + NLMSG_ALIGN(nlmsghdr->nlmsg_len);
> > + } else {
> > + *len = 0;
> > + return 0;
> > + }
> > +}
>
> Could it be written in a more clear and robust way? For example,
>
> unsigned long nlmsg_len = NLMSG_ALIGN(nlmsghdr->nlmsg_len);
>
> if (nlmsg_len < sizeof(struct nlmsghdr)) {
> addr = 0;
> nlmsg_len = sizeof(struct nlmsghdr);
> }
>
> if (*len >= nlmsg_len)
> *len -= nlmsg_len;
> else
> *len = 0;
>
> if (addr && addr + nlmsg_len > addr)
> return addr + nlmsg_len;
> else
> return 0;
Done.
> > +static void
> > +decode_netlink_msg(struct tcb *tcp, struct nlmsghdr *nlmsghdr,
> > + unsigned long addr, unsigned long size)
> > +{
> > if (size < sizeof(struct nlmsghdr)) {
> > printstr(tcp, addr, size);
> > return;
> > }
> > - if (umove_or_printaddr(tcp, addr, &nlmsghdr))
> > - return;
> >
> > - tprintf("{{len=%u, type=", nlmsghdr.nlmsg_len);
> > + tprintf("{{len=%u, type=", nlmsghdr->nlmsg_len);
> >
> > - printxval(netlink_types, nlmsghdr.nlmsg_type, "NLMSG_???");
> > + printxval(netlink_types, nlmsghdr->nlmsg_type, "NLMSG_???");
> >
> > tprints(", flags=");
> > - printflags(netlink_flags, nlmsghdr.nlmsg_flags, "NLM_F_???");
> > + printflags(netlink_flags, nlmsghdr->nlmsg_flags, "NLM_F_???");
> > /* manage get/new requests */
> >
> > - tprintf(", seq=%u, pid=%u}", nlmsghdr.nlmsg_seq,
> > - nlmsghdr.nlmsg_pid);
> > + tprintf(", seq=%u, pid=%u}", nlmsghdr->nlmsg_seq,
> > + nlmsghdr->nlmsg_pid);
> >
> > - if (size - sizeof(struct nlmsghdr) > 0) {
> > + if (nlmsghdr->nlmsg_len - sizeof(struct nlmsghdr) > 0) {
> > tprints(", ");
> > +
> > printstr(tcp, addr + sizeof(struct nlmsghdr),
> > - size - sizeof(struct nlmsghdr));
> > + nlmsghdr->nlmsg_len - sizeof(struct nlmsghdr));
> > }
> >
> > tprints("}");
> > }
> > +
> > +void
> > +decode_netlink(struct tcb *tcp, unsigned long addr, unsigned long
> > total_size) {
> > + struct nlmsghdr nlmsghdr;
> > + unsigned long elt, size = total_size;
> > + int print_array = 0;
> > +
> > + for (elt = 0; fetch_nlmsg(tcp, &nlmsghdr, addr, size);
> > + addr = next_nlmsg(&nlmsghdr, addr, &size), elt++) {
> > + if (elt == max_strlen && abbrev(tcp)) {
> > + tprints("...");
> > + break;
> > + }
> > + if (nlmsghdr.nlmsg_len < sizeof(struct nlmsghdr))
> > + break;
>
> This edition of decode_netlink will print nothing if addr == NULL,
> or if the first nlmsghdr.nlmsg_len < sizeof(struct nlmsghdr).
What should be printed in these cases?
>
> Please add to the test all pathological cases we discussed so far.
>
So let's add a test where nlmsghdr->nlmsg_len is 0 and a test with
nlmsghdr->nlmsg_len is greater than len. Do you have other pathological cases
in mind?
Cheers,
--
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
