<div dir="ltr">Hi,<br><br><div class="gmail_quote"><div dir="ltr">On Tue, Feb 16, 2016 at 12:21 PM Felix Fietkau <<a href="mailto:nbd@openwrt.org">nbd@openwrt.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 2016-02-16 11:13, Eyal Birger wrote:<br>
> Hi Felix, thanks for your patience.<br>
><br>
> On Tue, Feb 16, 2016 at 12:00 PM Felix Fietkau <<a href="mailto:nbd@openwrt.org" target="_blank">nbd@openwrt.org</a><br>
> <mailto:<a href="mailto:nbd@openwrt.org" target="_blank">nbd@openwrt.org</a>>> wrote:<br>
><br>
> On 2016-02-16 10:06, Eyal Birger wrote:<br>
> > Hi Felix,<br>
> ><br>
> > Thanks again for your responses.<br>
> ><br>
> > On Mon, Feb 15, 2016 at 2:14 PM Felix Fietkau <<a href="mailto:nbd@openwrt.org" target="_blank">nbd@openwrt.org</a><br>
> <mailto:<a href="mailto:nbd@openwrt.org" target="_blank">nbd@openwrt.org</a>><br>
> > <mailto:<a href="mailto:nbd@openwrt.org" target="_blank">nbd@openwrt.org</a> <mailto:<a href="mailto:nbd@openwrt.org" target="_blank">nbd@openwrt.org</a>>>> wrote:<br>
> ><br>
> > On 2016-02-15 12:54, Eyal Birger wrote:<br>
> > > > > if (offset < sizeof(ub->hdr)) {<br>
> > > > > - iov[0].iov_base = ((char *)<br>
> &ub->hdr) +<br>
> > offset;<br>
> > > > > - iov[0].iov_len = sizeof(ub->hdr) -<br>
> offset;<br>
> > > > > + struct ubus_msghdr hdr;<br>
> > > > > +<br>
> > > > > + hdr.version = ub->hdr.version;<br>
> > > > > + hdr.type = ub->hdr.type;<br>
> > > > > + hdr.seq = cpu_to_be16(ub->hdr.seq);<br>
> > > > > + hdr.peer = cpu_to_be32(ub->hdr.peer);<br>
> > > > > +<br>
> > > > > + iov[0].iov_base = ((char *) &hdr)<br>
> + offset;<br>
> > > > > + iov[0].iov_len = sizeof(hdr) - offset;<br>
> > > The corner case is this: You changed the iov to point at<br>
> stack<br>
> > space<br>
> > > instead of ub->hdr. If the code receives a part of the<br>
> header<br>
> > in one<br>
> > > call, and another one in the next (offset > 0), the contents<br>
> > of hdr will<br>
> > > be corrupt, as it will be a mix of uninitialized stack<br>
> space + the<br>
> > > received data from the last call.<br>
> > > Interesting... I initialize the iov_base every time to a newly<br>
> > created and<br>
> > > calculated hdr variable before the sendmsg() call, and iov is<br>
> > never used<br>
> > > otherwise - so I wonder how it could be reused in subsequent<br>
> calls?<br>
> > Before your change, iov[0].iov_base points at ub->hdr, which<br>
> is on heap<br>
> > and is preserved across calls.<br>
> > After your change, iov[0].iov_base points at the on-stack<br>
> struct hdr,<br>
> > which is not preserved across calls.<br>
> ><br>
> ><br>
> > The thing is, I don't see why the area pointed to in iov_base is<br>
> > required to be preserved between calls - the sendmsg() call never uses<br>
> > the cached values in iov. They are always re-armed with new pointers.<br>
><br>
> You're still confused, so please forget about iov for a second and only<br>
> focus on struct ubus_msghdr hdr. Think about what happens if the first<br>
> call only receives a few bytes of it, and the next call fills in the<br>
> rest.<br>
><br>
><br>
> Ok, I'm fine with the premise that I'm confused :)<br>
Sorry, I meant to write 'you're still confusing me'.<br>
<br>
> so I'll try to explain my confusion:<br>
><br>
> On the first call to ubus_msg_writev(offset = 0):<br>
> offset is less than sizeof(ub->hdr)<br>
> hdr is created on the stack, passed to sendmsg()<br>
> sendmsg() returns. No more references to hdr exist anywhere -<br>
> hdr contents is copied to the kernel socket for consumption.<br>
> but sendmsg() did not write all of the hdr (e.g. only 3 bytes).<br>
> ubus_msg_writev() returns with return value '3'.<br>
><br>
> Second call to ubus_msg_writev(offset = 3):<br>
> offset is still less than sizeof(ub->hdr)<br>
> hdr is recreated on the stack, it's a different hdr now that is sent<br>
> to sendmsg(), but contains the remaining bytes to be sent.<br>
><br>
> That's the point I don't understand - the old pointer to hdr is never used<br>
> again. So the only way this would create problems is if somehow the hdr<br>
> pointer is retained after the sendmsg() call exits. But calling<br>
> sendmsg() with iov's<br>
> pointing to stack area is legitimate as far as I can tell.<br>
It seems I got mixed up in reads vs writes and the fact that you<br>
re-create the buffer every time. It looks a bit convoluted, but it seems<br>
that it's fine after all. I will do a more thorough review of it to see<br>
if there is a simpler way to deal with it.<br></blockquote><div><br></div><div>Have you reached a conclusion on this?</div><div><br></div><div>Eyal.</div></div></div>