From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752592AbdJLRhA (ORCPT ); Thu, 12 Oct 2017 13:37:00 -0400 Received: from mail-pf0-f196.google.com ([209.85.192.196]:50696 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750722AbdJLRg6 (ORCPT ); Thu, 12 Oct 2017 13:36:58 -0400 X-Google-Smtp-Source: AOwi7QBR5XMJ5x63YwS1k9J4hK0Ax0HBrz14fpMAIvKuYRSNPV+E9a1xN75jE1ASZB5iL2715ee0Bw== Subject: Re: [PATCH] selftests/exec: include cwd in long path calculation To: David Drysdale Cc: Shuah Khan , linux-kselftest@vger.kernel.org, "linux-kernel@vger.kernel.org" References: <20171012004050.144564-1-smuckle.linux@gmail.com> From: Steve Muckle Message-ID: <8a8ae791-5c2c-a0b1-0f8f-83bad878b8f5@gmail.com> Date: Thu, 12 Oct 2017 10:36:55 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > On Thu, Oct 12, 2017 at 1:40 AM, Steve Muckle 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 >> --- >> 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