Hi, On Tue, Feb 16, 2016 at 12:21 PM Felix Fietkau <[email protected]> wrote:
> On 2016-02-16 11:13, Eyal Birger wrote: > > Hi Felix, thanks for your patience. > > > > On Tue, Feb 16, 2016 at 12:00 PM Felix Fietkau <[email protected] > > <mailto:[email protected]>> 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 <[email protected] > > <mailto:[email protected]> > > > <mailto:[email protected] <mailto:[email protected]>>> 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 :) > Sorry, I meant to write 'you're still confusing me'. > > > 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. > It seems I got mixed up in reads vs writes and the fact that you > re-create the buffer every time. It looks a bit convoluted, but it seems > that it's fine after all. I will do a more thorough review of it to see > if there is a simpler way to deal with it. > Have you reached a conclusion on this? Eyal.
_______________________________________________ openwrt-devel mailing list [email protected] https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
