On 3/26/21 12:05 PM, Rasmus Villemoes wrote: > On 26/03/2021 17.31, Alex Elder wrote: >> On 3/26/21 10:22 AM, Rasmus Villemoes wrote: >>> Including a nul byte in the otherwise human-readable ascii output >>> from this debugfs file is probably not intended. >> >> Looking only at the comments above simple_read_from_buffer(), >> the last argument is the size of the buffer (tmp_buf in this >> case). So "3" is proper. > > The size of the buffer is 3 because that's what sprintf() is gonna need > to print one digit, '\n' and a nul byte. That doesn't necessarily imply > that the entire buffer is meant to be sent to userspace. > >> But looking at callers, it seems that the trailing NUL is >> often excluded this way. >> >> I don't really have a problem with your patch, but could >> you explain why having the NUL byte included is an actual >> problem? A short statement about that would provide better >> context than just "probably not intended." My point was really that you should have provided a better explanation in your description. At this point it's been discussed enough so I won't ask you to post version 2. Acked-by: Alex Elder <elder@xxxxxxxxxx> > > debugfs files are AFAIK intended to be used with simple "cat foo", "echo > 1 > foo" in shell (scripts and interactive). Having non-printable > characters returned from that "cat foo" is odd (and can sometimes break > scripts that e.g. "grep 1 foo/*/*/bar" when grep stops because it thinks > one of the files is binary, or when the output of that is further piped > somewhere). > > At the very least, it's inconsistent for this one, those in > greybus/svc.c do just return the ascii digits and the newline (and if > one followed your argument above and let those pass 16 instead of desc > one would leak a few bytes of uninitialized kernel stack to userspace). > > I said "probably not intended" because for all I know, it might be > intentional. I just doubt it. > > Rasmus > _______________________________________________ greybus-dev mailing list greybus-dev@xxxxxxxxxxxxxxxx https://lists.linaro.org/mailman/listinfo/greybus-dev
|