[PATCH i-g-t v3 3/3] lib/igt_sysfs: make sure to write empty strings
Janusz Krzysztofik
janusz.krzysztofik at linux.intel.com
Wed Feb 28 16:07:52 UTC 2024
On Wednesday, 28 February 2024 16:54:59 CET Lucas De Marchi wrote:
> On Wed, Feb 28, 2024 at 12:13:22PM +0100, Janusz Krzysztofik wrote:
> >Hi Lucas,
> >
> >On Wednesday, 28 February 2024 07:32:11 CET Lucas De Marchi wrote:
> >> echo -n "" > /sys/module/<modulename>/parameters/<param>
> >>
> >> doesn't really work as it will just create a open() + close() expecting
> >> the file to be truncated. The same issue happens with igt as it will
> >> stop writing when there are 0 chars to write. Special case the empty
> >> string so it always write a '\0' and make sure igt_sysfs_set() accounts
> >> for the extra null char.
> >>
> >> Shell example:
> >> # echo -n "/foo" > /sys/module/firmware_class/parameters/path
> >> # cat /sys/module/firmware_class/parameters/path
> >> /foo
> >> # echo -n "" > /sys/module/firmware_class/parameters/path
> >> /foo
> >> # # make sure to actually write a \0:
> >> echo -ne "\0" > /sys/module/firmware_class/parameters/path
> >> # cat /sys/module/firmware_class/parameters/path
> >>
> >> Same thing happens when testing igt_sysfs_set():
> >> int dirfd = open("/sys/module/firmware_class/parameters", O_RDONLY);
> >> igt_sysfs_set(dirfd, "path", "");
> >>
> >> Previously it was not really setting the param.
> >>
> >> v2:
> >> - Fix return code from igt_sysfs_vprintf() to differentiate between
> >> writing 1 or 0 chars (Janusz)
> >> - Document the behavior of igt_sysfs_set() as being a higher-level
> >> than igt_sysfs_write().
> >> v3:
> >> - Partial "back to v1": include the \0 in the write only when writing
> >> an empty string: there are some files in sysfs like pm_test that
> >> don't like
> >>
> >> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> >> ---
> >> lib/igt_sysfs.c | 33 +++++++++++++++++++++++++++++----
> >> 1 file changed, 29 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> >> index 2997925e5..2d574cc17 100644
> >> --- a/lib/igt_sysfs.c
> >> +++ b/lib/igt_sysfs.c
> >> @@ -349,6 +349,9 @@ int igt_sysfs_get_num_gt(int device)
> >> * @len: the length to write
> >> *
> >> * This writes @len bytes from @data to the sysfs file.
> >> + * Contrary to igt_sysfs_set(), this does not automatically add a null char to
> >
> >Should now read something like "this does not automatically write a null char
> >if len is 0", I believe.
> >
> >> + * terminate the data. It's caller responsibility to pass the right len
> >> + * according to the data being written.
> >> *
> >> * Returns:
> >> * The number of bytes written, or -errno on error.
> >> @@ -407,6 +410,14 @@ int igt_sysfs_read(int dir, const char *attr, void *data, int len)
> >> bool igt_sysfs_set(int dir, const char *attr, const char *value)
> >> {
> >> int len = strlen(value);
> >> +
> >> + /*
> >> + * Always write at least 1 char, the null byte, otherwise it
> >> + * won't write anything on sysfs.
> >> + */
> >> + if (!len)
> >> + len = 1;
> >> +
> >> return igt_sysfs_write(dir, attr, value, len) == len;
> >> }
> >>
> >> @@ -506,7 +517,7 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
> >> {
> >> char stack[128], *buf = stack;
> >> va_list tmp;
> >> - int ret, fd;
> >> + int ret, fd, len;
> >>
> >> fd = openat(dir, attr, O_WRONLY);
> >> if (igt_debug_on(fd < 0))
> >> @@ -520,8 +531,18 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
> >> goto end;
> >> }
> >>
> >> - if (ret > sizeof(stack)) {
> >> - unsigned int len = ret + 1;
> >> + if (!ret) {
> >> + /*
> >> + * Make sure to always issue a write() syscall, even if writing
> >> + * an empty string, otherwise values in sysfs like module
> >> + * parameters don't really get overwritten. vsnprintf()
> >> + * guarantees to return a \0 terminated string, so just add
> >> + * that char. The return code is still the same as before, to
> >> + * abstract that from caller.
> >> + */
> >> + len = 1;
> >> + } else if (ret > sizeof(stack)) {
> >> + len = ret + 1;
> >>
> >> buf = malloc(len);
> >> if (igt_debug_on(!buf)) {
> >> @@ -536,7 +557,11 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
> >> }
> >> }
> >>
> >> - ret = igt_writen(fd, buf, ret);
> >> + ret = igt_writen(fd, buf, len);
> >> +
> >> + /* Caller shouldn't know about special sysfs handling, just return 0 */
> >> + if (ret == 1 && buf[0] == '\0')
> >> + ret = 0;
> >
> >Hmm, now we no longer support writing data with null character embedded at
> >position 0.
>
> why not?
>
> igt_sysfs_vprintf(fd, "foo", "") will cause vsnprintf() to return 0.
> igt_sysfs_vprintf(fd, "foo", "\0") will cause vsnprintf() to return 0.
> [1]
>
> which is also true for both if tried like
>
> foo_val = "\0";
> igt_sysfs_vprintf(fd, "foo", "%s", foo_val);
>
> Is this what you are trying to differentiate or did I misunderstand?
Not with just "%s" (you better use igt_sysfs_set() for that), but possible
with e.g.
igt_sysfs_printf(fd, attr, "%c<more_fmt>", c, ...);
Thanks,
Janusz
>
> there's no difference from printf/snprintf point of view, so expecting a
> return > 0 when appending \0 would be wrong.
> ( Could also be considered bad code to use *printf() family of function
> to any constant string )
>
> The patch seems to already handle that correctly and return 0 like snprintf()
> would as far as I can see.
>
>
> Lucas De Marchi
>
> [ 1 ]
> #include <stdio.h>
>
> char buf[10];
>
> int main(int argc, const char *argv[])
> {
> int ret;
> char *foo_val = "";
> char *foo_val2 = "\0";
> char *foo_val3 = "\0test";
>
> ret = snprintf(buf, sizeof(buf), "");
> printf("ret=%d\n", ret);
>
> ret = snprintf(buf, sizeof(buf), "\0");
> printf("ret=%d\n", ret);
>
> ret = snprintf(buf, sizeof(buf), "\0\0\0");
> printf("ret=%d\n", ret);
>
> ret = snprintf(buf, sizeof(buf), "%s", foo_val);
> printf("ret=%d\n", ret);
>
> ret = snprintf(buf, sizeof(buf), "%s", foo_val2);
> printf("ret=%d\n", ret);
>
> ret = snprintf(buf, sizeof(buf), "%s", foo_val3);
> printf("ret=%d\n", ret);
>
> return 0;
> }
>
> $ /tmp/a
> ret=0
> ret=0
> ret=0
> ret=0
> ret=0
> ret=0
>
> btw, clang also warns by default about using printf like that:
>
> $ clang -o /tmp/a /tmp/a.c && /tmp/a
> /tmp/a.c:15:36: warning: format string contains '\0' within the string body [-Wformat]
> ret = snprintf(buf, sizeof(buf), "\0");
> ~^~~
> /tmp/a.c:18:36: warning: format string contains '\0' within the string body [-Wformat]
> ret = snprintf(buf, sizeof(buf), "\0\0\0");
> ~^~~~~~~
> 2 warnings generated.
>
>
> and gcc with -Wall goes a little bit further and also warns on the
> empty string.
> /tmp/a.c: In function ‘main’:
> /tmp/a.c:12:42: warning: zero-length gnu_printf format string [-Wformat-zero-length]
> 12 | ret = snprintf(buf, sizeof(buf), "");
> | ^~
> /tmp/a.c:15:43: warning: embedded ‘\0’ in format [-Wformat-contains-nul]
> 15 | ret = snprintf(buf, sizeof(buf), "\0");
> | ^~
> /tmp/a.c:18:43: warning: embedded ‘\0’ in format [-Wformat-contains-nul]
> 18 | ret = snprintf(buf, sizeof(buf), "\0\0\0");
> |
>
> Lucas De Marchi
>
> >
> >How about keeping the original length of the resulting string in len?
> >Seems easy to get with your code slightly modified:
> >
> >
> > - if (ret > sizeof(stack)) {
> > - unsigned int len = ret + 1;
> >++ len = ret;
> > + if (!ret)
> > + /*
> > + * Make sure to always issue a write() syscall, even if writing
> > + * an empty string, otherwise values in sysfs like module
> > + * parameters don't really get overwritten. vsnprintf()
> > + * guarantees to return a \0 terminated string, so just add
> > + * that char. The return code is still the same as before, to
> > + * abstract that from caller.
> > + */
> > + ret = 1;
> > + } else if (ret > sizeof(stack)) {
> >-+ len = ret + 1;
> >-
> >- buf = malloc(len);
> >+-
> >+- buf = malloc(len);
> >++ buf = malloc(len + 1);
> > if (igt_debug_on(!buf)) {
> >+ ret = -ENOMEM;
> >+ goto end;
> >+ }
> >+
> >+ ret = vsnprintf(buf, ret, fmt, ap);
> >+- if (igt_debug_on(ret > len)) {
> >++ if (igt_debug_on(ret > len + 1)) {
> >+ ret = -EINVAL;
> >+ goto free_buf;
> >+ }
> >
> >and then:
> >
> > }
> >
> >-- ret = igt_writen(fd, buf, ret);
> >-+ ret = igt_writen(fd, buf, len);
> >+ ret = igt_writen(fd, buf, ret);
> > +
> > + /* Caller shouldn't know about special sysfs handling, just return 0 */
> >-+ if (ret == 1 && buf[0] == '\0')
> >++ if (!len && ret == 1)
> > + ret = 0;
> >
> >Thanks,
> >Janusz
> >
> >
> >>
> >> free_buf:
> >> if (buf != stack)
> >>
> >
> >
> >
> >
>
More information about the igt-dev
mailing list