Re: [PATCH] selftests/exec: include cwd in long path calculation

[Steve Muckle]

Thanks David for the review. Replies inline.

On 10/12/2017 05:22 AM, David Drysdale wrote:
> Modulo the minor comment below:
>
> Reviewed-by: David Drysdale <drysdale@xxxxxxxxxx>
>
> On Thu, Oct 12, 2017 at 1:40 AM, Steve Muckle <smuckle.linux@xxxxxxxxx> wrote:
> > When creating a pathname close to PATH_MAX to test execveat, factor in
> > the current working directory path otherwise we end up with an absolute
> > path that is longer than PATH_MAX. While execveat() may succeed, subsequent
> > calls to the kernel from the runtime environment which are required to
> > successfully execute the test binary/script may fail because of this.
> >
> > To keep the semantics of the test the same, rework the relative pathname
> > part of the test to be relative to the root directory so it isn't
> > decreased by the length of the current working directory path.
> >
> > Signed-off-by: Steve Muckle <smuckle.linux@xxxxxxxxx>
> > ---
> >   tools/testing/selftests/exec/execveat.c | 24 +++++++++++++++++-------
> >   1 file changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/testing/selftests/exec/execveat.c b/tools/testing/selftests/exec/execveat.c
> > index 8d5d1d2ee7c1..5edc609c778b 100644
> > --- a/tools/testing/selftests/exec/execveat.c
> > +++ b/tools/testing/selftests/exec/execveat.c
> > @@ -147,7 +147,7 @@ static void exe_cp(const char *src, const char *dest)
> >   }
> >
> >   #define XX_DIR_LEN 200
> > -static int check_execveat_pathmax(int dot_dfd, const char *src, int is_script)
> > +static int check_execveat_pathmax(int root_dfd, const char *src, int is_script)
> >   {
> >          int fail = 0;
> >          int ii, count, len;
> > @@ -156,15 +156,24 @@ static int check_execveat_pathmax(int dot_dfd, const char *src, int is_script)
> >
> >          if (*longpath == '\0') {
> >                  /* Create a filename close to PATH_MAX in length */
> > +               char *cwd = getcwd(NULL, 0);
>
> cwd never gets freed, but that's presumably not a problem unless these
> tests ever get run under some kind of leak checker.

Yeah I found some other instances of this in the test (specifically the 
calls to realpath() at the top of run_tests() which alloc their own 
buffer) and figured for a small unit test like this it had been decided 
it wasn't worth freeing stuff. I'll tidy this up anyway though.

> > +
> > +               if (!cwd) {
> > +                       printf("Failed to getcwd(), errno=%d (%s)\n",
> > +                              errno, strerror(errno));
> > +                       return 2;
> > +               }
> > +               strcpy(longpath, cwd);
> > +               strcat(longpath, "/");
> >                  memset(longname, 'x', XX_DIR_LEN - 1);
> >                  longname[XX_DIR_LEN - 1] = '/';
> >                  longname[XX_DIR_LEN] = '\0';
> > -               count = (PATH_MAX - 3) / XX_DIR_LEN;
> > +               count = (PATH_MAX - 3 - strlen(cwd)) / XX_DIR_LEN;
> >                  for (ii = 0; ii < count; ii++) {
> >                          strcat(longpath, longname);
> >                          mkdir(longpath, 0755);
> >                  }
> > -               len = (PATH_MAX - 3) - (count * XX_DIR_LEN);
> > +               len = (PATH_MAX - 3 - strlen(cwd)) - (count * XX_DIR_LEN);
> >                  if (len <= 0)
> >                          len = 1;
> >                  memset(longname, 'y', len);
>
> There's a missing comment update around here-ish:
>    * Execute as a long pathname relative to ".".
> (should now be 'relative to "/"' I think).

Will fix.

thanks,
Steve