[OpenWrt-Devel] [PATCH v2] ubus: use network order in ubus message header fields

[Eyal Birger]

Hi Felix, thanks for your patience.

On Tue, Feb 16, 2016 at 12:00 PM Felix Fietkau <nbd at openwrt.org> wrote:

> On 2016-02-16 10:06, Eyal Birger wrote:
> > Hi Felix,
> >
> > Thanks again for your responses.
> >
> > On Mon, Feb 15, 2016 at 2:14 PM Felix Fietkau <nbd at openwrt.org
> > <mailto:nbd at openwrt.org>> wrote:
> >
> >     On 2016-02-15 12:54, Eyal Birger wrote:
> >     >     >     >       if (offset < sizeof(ub->hdr)) {
> >     >     >     > -             iov[0].iov_base = ((char *) &ub->hdr) +
> >     offset;
> >     >     >     > -             iov[0].iov_len = sizeof(ub->hdr) -
> offset;
> >     >     >     > +             struct ubus_msghdr hdr;
> >     >     >     > +
> >     >     >     > +             hdr.version = ub->hdr.version;
> >     >     >     > +             hdr.type = ub->hdr.type;
> >     >     >     > +             hdr.seq = cpu_to_be16(ub->hdr.seq);
> >     >     >     > +             hdr.peer = cpu_to_be32(ub->hdr.peer);
> >     >     >     > +
> >     >     >     > +             iov[0].iov_base = ((char *) &hdr) +
> offset;
> >     >     >     > +             iov[0].iov_len = sizeof(hdr) - offset;
> >     >     The corner case is this: You changed the iov to point at stack
> >     space
> >     >     instead of ub->hdr. If the code receives a part of the header
> >     in one
> >     >     call, and another one in the next (offset > 0), the contents
> >     of hdr will
> >     >     be corrupt, as it will be a mix of uninitialized stack space +
> the
> >     >     received data from the last call.
> >     > Interesting... I initialize the iov_base every time to a newly
> >     created and
> >     > calculated hdr variable before the sendmsg() call, and iov is
> >     never used
> >     > otherwise - so I wonder how it could be reused in subsequent calls?
> >     Before your change, iov[0].iov_base points at ub->hdr, which is on
> heap
> >     and is preserved across calls.
> >     After your change, iov[0].iov_base points at the on-stack struct hdr,
> >     which is not preserved across calls.
> >
> >
> > The thing is, I don't see why the area pointed to in iov_base is
> > required to be preserved between calls - the sendmsg() call never uses
> > the cached values in iov. They are always re-armed with new pointers.
>
> You're still confused, so please forget about iov for a second and only
> focus on struct ubus_msghdr hdr. Think about what happens if the first
> call only receives a few bytes of it, and the next call fills in the rest.
>

Ok, I'm fine with the premise that I'm confused :)

so I'll try to explain my confusion:

On the first call to ubus_msg_writev(offset = 0):
  offset is less than sizeof(ub->hdr)
    hdr is created on the stack, passed to sendmsg()
    sendmsg() returns. No more references to hdr exist anywhere -
      hdr contents is copied to the kernel socket for consumption.
    but sendmsg() did not write all of the hdr (e.g. only 3 bytes).
    ubus_msg_writev() returns with return value '3'.

Second call to ubus_msg_writev(offset = 3):
  offset is still less than sizeof(ub->hdr)
    hdr is recreated on the stack, it's a different hdr now that is sent
        to sendmsg(), but contains the remaining bytes to be sent.

That's the point I don't understand - the old pointer to hdr is never used
again. So the only way this would create problems is if somehow the hdr
pointer is retained after the sendmsg() call exits. But calling sendmsg()
with iov's
pointing to stack area is legitimate as far as I can tell.

Eyal.






>
> - Felix
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/openwrt-devel/attachments/20160216/e6810e52/attachment.htm>
-------------- next part --------------
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel