[PATCH] Fix segmentation fault in test_io.

[Lukas Slebodnik]

I wanted to revie som patches, but test failed with unrelated issue to revieved
patch.

------------------------------------------
[ RUN      ] test_sss_open_cloexec_success
mkstemp failed
fd != -1
ERROR: ../sssd/src/tests/cmocka/test_io.c:81 Failure!
[  FAILED  ] test_sss_open_cloexec_success
[ RUN      ] test_sss_open_cloexec_fail
[       OK ] test_sss_open_cloexec_fail
[ RUN      ] test_sss_openat_cloexec_success
mkstemp failed
11
[  FAILED  ] test_sss_openat_cloexec_success
[ RUN      ] test_sss_openat_cloexec_fail
/bin/sh: line 5: 12545 Segmentation fault      (core dumped)     LDB_MODULES_PATH=/dev/shm/sssd_build/ldb_mod_test_dir ${dir}$tst 
FAIL: test-io
------------------------------------------

Attached patch fix this bug.

LS

[Jakub Hrozek]

On Thu, Apr 11, 2013 at 11:04:06AM +0200, Lukas Slebodnik wrote:
> I wanted to revie som patches, but test failed with unrelated issue to revieved
> patch.

The test_io.c test should also call tests_set_cwd().

Is this only an issue with test_io.c ? Did sysdb_tests run OK? I would
expect them to have the same problem, can you check?

[Lukas Slebodnik]

On (11/04/13 11:18), Jakub Hrozek wrote:
> On Thu, Apr 11, 2013 at 11:04:06AM +0200, Lukas Slebodnik wrote:
> > I wanted to revie som patches, but test failed with unrelated issue to revieved
> > patch.
>
> The test_io.c test should also call tests_set_cwd().
>
> Is this only an issue with test_io.c ? Did sysdb_tests run OK? I would
> expect them to have the same problem, can you check?

I fixed also tests_set_cwd(), there was ignored return value if TEST_DIR is empty
string. It should not happen with default TEST_DIR value.

Attached patch just fix segmentation fault and improve some lines to prevent
segfault.

Abhishek can rewrite test_io.c to using tests_set_cwd(),
(use relative paths)
Also there something what is not very good solution (transformation to relativepath)

relativepath = strchr(get_filepath(path), 't');
                                             ^^^^^
What if I decide to call ./configure --with-test-dir="temp_test"?

LS

[Jakub Hrozek]

On Thu, Apr 11, 2013 at 01:47:03PM +0200, Lukas Slebodnik wrote:
> On (11/04/13 11:18), Jakub Hrozek wrote:
> > On Thu, Apr 11, 2013 at 11:04:06AM +0200, Lukas Slebodnik wrote:
> > > I wanted to revie som patches, but test failed with unrelated issue to revieved
> > > patch.
> >
> > The test_io.c test should also call tests_set_cwd().
> >
> > Is this only an issue with test_io.c ? Did sysdb_tests run OK? I would
> > expect them to have the same problem, can you check?
>
> I fixed also tests_set_cwd(), there was ignored return value if TEST_DIR is empty
> string. It should not happen with default TEST_DIR value.
>
> Attached patch just fix segmentation fault and improve some lines to prevent
> segfault.
>
> Abhishek can rewrite test_io.c to using tests_set_cwd(),
> (use relative paths)
> Also there something what is not very good solution (transformation to relativepath)
> relativepath = strchr(get_filepath(path), 't');
>                                          ^^^^^
>
> What if I decide to call ./configure --with-test-dir="temp_test"?
>
> LS

Lukas, please send a patch that would fix the above with calling basename,
please. We need to fix this in master asap.

[Abhishek Singh]

Required changes done. Please check the patch.

On Fri, Apr 12, 2013 at 9:28 PM, Jakub Hrozek  wrote:
> On Thu, Apr 11, 2013 at 01:47:03PM +0200, Lukas Slebodnik wrote:
> > On (11/04/13 11:18), Jakub Hrozek wrote:
> > > On Thu, Apr 11, 2013 at 11:04:06AM +0200, Lukas Slebodnik wrote:
> > > > I wanted to revie som patches, but test failed with unrelated issue
>
> to revieved
> > > > patch.
> > >
> > > The test_io.c test should also call tests_set_cwd().
> > >
> > > Is this only an issue with test_io.c ? Did sysdb_tests run OK? I would
> > > expect them to have the same problem, can you check?
> >
> > I fixed also tests_set_cwd(), there was ignored return value if TEST_DIR
>
> is empty
> > string. It should not happen with default TEST_DIR value.
> >
> > Attached patch just fix segmentation fault and improve some lines to
>
> prevent
> > segfault.
> >
> > Abhishek can rewrite test_io.c to using tests_set_cwd(),
> > (use relative paths)
> > Also there something what is not very good solution (transformation to
>
> relativepath)
> > relativepath = strchr(get_filepath(path), 't');
> >                                          ^^^^^
> >
> > What if I decide to call ./configure --with-test-dir="temp_test"?
> >
> > LS
>
> Lukas, please send a patch that would fix the above with calling basename,
> please. We need to fix this in master asap.

[Abhishek Singh]

Please check this patch, previous patch contained extra changes which is
not required.

On Sat, Apr 13, 2013 at 2:30 AM, Abhishek Singh <
[e-mail]> wrote:
> Required changes done. Please check the patch.
>
> On Fri, Apr 12, 2013 at 9:28 PM, Jakub Hrozek  wrote:
> > On Thu, Apr 11, 2013 at 01:47:03PM +0200, Lukas Slebodnik wrote:
> > > On (11/04/13 11:18), Jakub Hrozek wrote:
> > > > On Thu, Apr 11, 2013 at 11:04:06AM +0200, Lukas Slebodnik wrote:
> > > > > I wanted to revie som patches, but test failed with unrelated issue
> >
> > to revieved
> > > > > patch.
> > > >
> > > > The test_io.c test should also call tests_set_cwd().
> > > >
> > > > Is this only an issue with test_io.c ? Did sysdb_tests run OK? I would
> > > > expect them to have the same problem, can you check?
> > >
> > > I fixed also tests_set_cwd(), there was ignored return value if
> >
> > TEST_DIR is empty
> > > string. It should not happen with default TEST_DIR value.
> > >
> > > Attached patch just fix segmentation fault and improve some lines to
> >
> > prevent
> > > segfault.
> > >
> > > Abhishek can rewrite test_io.c to using tests_set_cwd(),
> > > (use relative paths)
> > > Also there something what is not very good solution (transformation to
> >
> > relativepath)
> > > relativepath = strchr(get_filepath(path), 't');
> > >                                          ^^^^^
> > >
> > > What if I decide to call ./configure --with-test-dir="temp_test"?
> > >
> > > LS
> >
> > Lukas, please send a patch that would fix the above with calling basename,
> > please. We need to fix this in master asap.

[Jakub Hrozek]

On Sat, Apr 13, 2013 at 02:38:45AM +0530, Abhishek Singh wrote:
> Please check this patch, previous patch contained extra changes which is
> not required.

I would prefer if the src/conf_macros.m4 and src/tests/common.c changes
were a separate patch (with attribution set to Lukas) But more importantly:

@@ -113,8 +121,8 @@ void test_sss_openat_cloexec_success(void **state)
     char path[PATH_MAX] = {'\0'};
     const char *relativepath;

-    relativepath = strchr(get_filepath(path), 't');
     dir_fd = dirfd((DIR *)*state);
+    relativepath = get_filepath(path) + strlen(TEST_DIR) + 1;

This is still guesswork, can you simply use basename() here? See man 3 basename,
I think you could easily call basename() on the return value of get_filepath().

Also the test still doesn't call tests_set_cwd(). It should call it right before
calling run_tests, I think.

[Abhishek Singh]

Hi Jakub,

I am bit puzzled here, since all the test files get created/destroyed under
TEST_DIR and runs well(absolute paths are already provided), so why do we
need to change current working dir to TEST_DIR (use of tests_set_cwd)
before running test?

On Mon, Apr 15, 2013 at 3:58 PM, Jakub Hrozek  wrote:
> On Sat, Apr 13, 2013 at 02:38:45AM +0530, Abhishek Singh wrote:
> > Please check this patch, previous patch contained extra changes which is
> > not required.
>
> I would prefer if the src/conf_macros.m4 and src/tests/common.c changes
> were a separate patch (with attribution set to Lukas) But more importantly:
>
> @@ -113,8 +121,8 @@ void test_sss_openat_cloexec_success(void **state)
>      char path[PATH_MAX] = {'\0'};
>      const char *relativepath;
> relativepath = strchr(get_filepath(path), 't');
> dir_fd = dirfd((DIR *)*state);
> relativepath = get_filepath(path) + strlen(TEST_DIR) + 1;
>
> This is still guesswork, can you simply use basename() here? See man 3
> basename,
> I think you could easily call basename() on the return value of
> get_filepath().
>
> Also the test still doesn't call tests_set_cwd(). It should call it right
> before
> calling run_tests, I think.

[Jakub Hrozek]

On Mon, Apr 15, 2013 at 05:46:25PM +0530, Abhishek Singh wrote:
> Hi Jakub,
>
> I am bit puzzled here, since all the test files get created/destroyed under
> TEST_DIR and runs well(absolute paths are already provided), so why do we
> need to change current working dir to TEST_DIR (use of tests_set_cwd)
> before running test?

Mostly as an additional precation or in case the tests had some kind of
side-effect.
> On Mon, Apr 15, 2013 at 3:58 PM, Jakub Hrozek  wrote:
> > On Sat, Apr 13, 2013 at 02:38:45AM +0530, Abhishek Singh wrote:
> > > Please check this patch, previous patch contained extra changes which is
> > > not required.
> >
> > I would prefer if the src/conf_macros.m4 and src/tests/common.c changes
> > were a separate patch (with attribution set to Lukas) But more importantly:
> >
> > @@ -113,8 +121,8 @@ void test_sss_openat_cloexec_success(void **state)
> >      char path[PATH_MAX] = {'\0'};
> >      const char *relativepath;
> > relativepath = strchr(get_filepath(path), 't');
> > dir_fd = dirfd((DIR *)*state);
> > relativepath = get_filepath(path) + strlen(TEST_DIR) + 1;
> >
> > This is still guesswork, can you simply use basename() here? See man 3
> > basename,
> > I think you could easily call basename() on the return value of
> > get_filepath().
> >
> > Also the test still doesn't call tests_set_cwd(). It should call it right
> > before
> > calling run_tests, I think.

[Abhishek Singh]

Changes done. Please check the patch.

thanks,

On Tue, Apr 16, 2013 at 4:19 PM, Jakub Hrozek  wrote:
> On Mon, Apr 15, 2013 at 05:46:25PM +0530, Abhishek Singh wrote:
> > Hi Jakub,
> >
> > I am bit puzzled here, since all the test files get created/destroyed
>
> under
> > TEST_DIR and runs well(absolute paths are already provided), so why do we
> > need to change current working dir to TEST_DIR (use of tests_set_cwd)
> > before running test?
>
> Mostly as an additional precation or in case the tests had some kind of
> side-effect.
> > On Mon, Apr 15, 2013 at 3:58 PM, Jakub Hrozek
>
> wrote:
> > > On Sat, Apr 13, 2013 at 02:38:45AM +0530, Abhishek Singh wrote:
> > > > Please check this patch, previous patch contained extra changes
>
> which is
> > > > not required.
> > >
> > > I would prefer if the src/conf_macros.m4 and src/tests/common.c changes
> > > were a separate patch (with attribution set to Lukas) But more
>
> importantly:
> > > @@ -113,8 +121,8 @@ void test_sss_openat_cloexec_success(void **state)
> > >      char path[PATH_MAX] = {'\0'};
> > >      const char *relativepath;
> > > relativepath = strchr(get_filepath(path), 't');
> > > dir_fd = dirfd((DIR *)*state);
> > > relativepath = get_filepath(path) + strlen(TEST_DIR) + 1;
> > >
> > > This is still guesswork, can you simply use basename() here? See man 3
> > > basename,
> > > I think you could easily call basename() on the return value of
> > > get_filepath().
> > >
> > > Also the test still doesn't call tests_set_cwd(). It should call it
>
> right
> > > before
> > > calling run_tests, I think.

[Jakub Hrozek]

On Tue, Apr 16, 2013 at 07:27:20PM +0530, Abhishek Singh wrote:
> Changes done. Please check the patch.
>
> thanks,

Hi Abishek

let's do one last change before I push the patch -- test the strdup
output for being non-NULL and then free the duplicate when it's not
needed:

basec = strdup(get_filepath(path));
+   assert_non_null(basec);
    relativepath = basename(basec);
    fd = sss_openat_cloexec(dir_fd, relativepath, flags, &ret);
+   free(basec)

Thanks for the patch!

[Abhishek Singh]

changes done. Please check the patch.

On Mon, Apr 22, 2013 at 12:55 PM, Jakub Hrozek  wrote:
> On Tue, Apr 16, 2013 at 07:27:20PM +0530, Abhishek Singh wrote:
> > Changes done. Please check the patch.
> >
> > thanks,
>
> Hi Abishek
>
> let's do one last change before I push the patch -- test the strdup
> output for being non-NULL and then free the duplicate when it's not
> needed:
> basec = strdup(get_filepath(path));
> assert_non_null(basec);
> relativepath = basename(basec);
> fd = sss_openat_cloexec(dir_fd, relativepath, flags, &ret);
> free(basec)
>
> Thanks for the patch!

[Jakub Hrozek]

On Mon, Apr 22, 2013 at 05:29:05PM +0530, Abhishek Singh wrote:
> changes done. Please check the patch.

Looks good to me now.

Ack!
> On Mon, Apr 22, 2013 at 12:55 PM, Jakub Hrozek  wrote:
> > On Tue, Apr 16, 2013 at 07:27:20PM +0530, Abhishek Singh wrote:
> > > Changes done. Please check the patch.
> > >
> > > thanks,
> >
> > Hi Abishek
> >
> > let's do one last change before I push the patch -- test the strdup
> > output for being non-NULL and then free the duplicate when it's not
> > needed:
> > basec = strdup(get_filepath(path));
> > assert_non_null(basec);
> > relativepath = basename(basec);
> > fd = sss_openat_cloexec(dir_fd, relativepath, flags, &ret);
> > free(basec)
> >
> > Thanks for the patch!

[Jakub Hrozek]

On Wed, Apr 24, 2013 at 09:35:40PM +0200, Jakub Hrozek wrote:
> On Mon, Apr 22, 2013 at 05:29:05PM +0530, Abhishek Singh wrote:
> > changes done. Please check the patch.
>
> Looks good to me now.
>
> Ack!

I reworded the commit message of the first patch and pushed both patches
to master.