| Message ID | 20230118133643.11371-1-chrubis@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | syscalls/statvfs01: Correcly zero terminate the strings | expand |
Hi Cyril,
I'm sorry not catching these, thanks for fixing.
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Kind regards,
Petr
Hi, On 18. 01. 23 14:36, Cyril Hrubis wrote: > Fixes random failures caused by the fact that the stack is not > guaranteed to be zeroed. > > Fixes: e305ac4a305f ("statvfs01: Convert to new LTP API") > Signed-off-by: Cyril Hrubis <chrubis@suse.cz> > CC: Petr Vorel <pvorel@suse.cz> > CC: Richard Palethorpe <rpalethorpe@suse.com> > CC: Avinesh Kumar <akumar@suse.de> > --- > testcases/kernel/syscalls/statvfs/statvfs01.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/testcases/kernel/syscalls/statvfs/statvfs01.c b/testcases/kernel/syscalls/statvfs/statvfs01.c > index dd14d6a0e..f17dc4cfa 100644 > --- a/testcases/kernel/syscalls/statvfs/statvfs01.c > +++ b/testcases/kernel/syscalls/statvfs/statvfs01.c > @@ -38,6 +38,9 @@ static void run(void) > memset(valid_fname, 'a', valid_len); > memset(toolong_fname, 'b', valid_len + 1); > > + valid_fname[valid_len] = 0; > + toolong_fname[valid_len+1] = 0; Is there a possibility that valid_len could be equal to PATH_MAX-1? I think toolong_fname[] size should be bumped to at least PATH_MAX+1. > + > TST_EXP_FD(creat(valid_fname, 0444)); > SAFE_CLOSE(TST_RET); >
> Hi, > On 18. 01. 23 14:36, Cyril Hrubis wrote: > > Fixes random failures caused by the fact that the stack is not > > guaranteed to be zeroed. > > Fixes: e305ac4a305f ("statvfs01: Convert to new LTP API") > > Signed-off-by: Cyril Hrubis <chrubis@suse.cz> > > CC: Petr Vorel <pvorel@suse.cz> > > CC: Richard Palethorpe <rpalethorpe@suse.com> > > CC: Avinesh Kumar <akumar@suse.de> > > --- > > testcases/kernel/syscalls/statvfs/statvfs01.c | 3 +++ > > 1 file changed, 3 insertions(+) > > diff --git a/testcases/kernel/syscalls/statvfs/statvfs01.c b/testcases/kernel/syscalls/statvfs/statvfs01.c > > index dd14d6a0e..f17dc4cfa 100644 > > --- a/testcases/kernel/syscalls/statvfs/statvfs01.c > > +++ b/testcases/kernel/syscalls/statvfs/statvfs01.c > > @@ -38,6 +38,9 @@ static void run(void) > > memset(valid_fname, 'a', valid_len); > > memset(toolong_fname, 'b', valid_len + 1); > > + valid_fname[valid_len] = 0; > > + toolong_fname[valid_len+1] = 0; > Is there a possibility that valid_len could be equal to PATH_MAX-1? I think PATH_MAX is 4096. statvfs.f_namemax is mostly 255 (only for vfat and exfat is 1530, but that's for multibyte names, the "real" length, when counting characters is also 255). > toolong_fname[] size should be bumped to at least PATH_MAX+1. statvfs01.c:43:22: warning: array subscript 4097 is above array bounds of ‘char[4096]’ [-Warray-bounds] => PATH_MAX-1 is the maximum (array size is PATH_MAX). Kind regards, Petr
Hi! > > memset(valid_fname, 'a', valid_len); > > memset(toolong_fname, 'b', valid_len + 1); > > > > + valid_fname[valid_len] = 0; > > + toolong_fname[valid_len+1] = 0; > > Is there a possibility that valid_len could be equal to PATH_MAX-1? I do not think so, POSIX explicitly says that PATH_MAX should include space for terminating null character. Btw there is NAME_MAX constant that is defined to 256 which is the usuall maximal lenght + 1. > I think toolong_fname[] size should be bumped to at least PATH_MAX+1. Well we can do that (in a separate patch) to be extra sure, but even then the PATH_MAX seems to be at least order of magnitude larger than NAME_MAX.
On 19. 01. 23 15:41, Cyril Hrubis wrote: > Hi! >>> memset(valid_fname, 'a', valid_len); >>> memset(toolong_fname, 'b', valid_len + 1); >>> >>> + valid_fname[valid_len] = 0; >>> + toolong_fname[valid_len+1] = 0; >> >> Is there a possibility that valid_len could be equal to PATH_MAX-1? > > I do not think so, POSIX explicitly says that PATH_MAX should include > space for terminating null character. That's still true if valid_len == PATH_MAX-1. But we write valid_len+2 bytes into toolong_fname. But if always f_namemax <<< PATH_MAX, then we don't need to do anything.
diff --git a/testcases/kernel/syscalls/statvfs/statvfs01.c b/testcases/kernel/syscalls/statvfs/statvfs01.c index dd14d6a0e..f17dc4cfa 100644 --- a/testcases/kernel/syscalls/statvfs/statvfs01.c +++ b/testcases/kernel/syscalls/statvfs/statvfs01.c @@ -38,6 +38,9 @@ static void run(void) memset(valid_fname, 'a', valid_len); memset(toolong_fname, 'b', valid_len + 1); + valid_fname[valid_len] = 0; + toolong_fname[valid_len+1] = 0; + TST_EXP_FD(creat(valid_fname, 0444)); SAFE_CLOSE(TST_RET);
Fixes random failures caused by the fact that the stack is not guaranteed to be zeroed. Fixes: e305ac4a305f ("statvfs01: Convert to new LTP API") Signed-off-by: Cyril Hrubis <chrubis@suse.cz> CC: Petr Vorel <pvorel@suse.cz> CC: Richard Palethorpe <rpalethorpe@suse.com> CC: Avinesh Kumar <akumar@suse.de> --- testcases/kernel/syscalls/statvfs/statvfs01.c | 3 +++ 1 file changed, 3 insertions(+)