Re: [LTP] [PATCH] Fix FILECAPS test hanging for more than 12 hours

[Garrett C. <yan...@gm...>]

On May 5, 2010, at 7:18 AM, Serge E. Hallyn wrote:

> Quoting Garrett Cooper (yan...@gm...):
>>>                p = index(buf, '.')+1;
> 
> Jinkeys!  The intertubes archives insist I wrote that, but I'm finding
> it hard to believe.
> 
>>> -               if (p==(char *)1) {
>>> -                       tst_resm(TFAIL, "got a bad message from print_caps\n");
>>> -                       tst_exit();
>>> -               }
>>> +               if (p==(char *)1)
>>> +                       tst_brkm(TFAIL, tst_exit, "got a bad message from print_caps\n");
>> 
>>    This is a really incorrect way to do things. I think that the
>> assumption made was that index(3) would return 0 ('\0') if it fails to
>> find '.'. That's incorrect and would cause a segfault on some systems
>> (does on FreeBSD at least... don't see why it would pass on Linux):
>> 
>> $ ~/test_null_inc
>> Segmentation fault: 11 (core dumped)
>> [garrcoop@bioshock ~]$ cat ~/test_null_inc.c
>> #include <stdio.h>
>> int
>> main(void)
>> {
>> 	printf("%s\n", (NULL + 1));
>> 	return 0;
>> }
> 
> Well, that's different - you're dereferencing NULL+1, whereas I'm
> just checking the the value of the pointer.  
> 
> Still what I did is darned ugly, cleanup below.
> 
> thanks,
> -serge
> 
>>    Could you please change this to check and see whether or not index
>> returns NULL instead of accessing memory like that?
>>    Other than that, patch looks good.
> 
> From: Serge E. Hallyn <se...@us...>
> Date: Wed, 5 May 2010 02:59:05 -0500
> Subject: [PATCH 1/1] check for index(3) returning NULL
> 
> Signed-off-by: Serge E. Hallyn <se...@us...>
> ---
> .../kernel/security/filecaps/verify_caps_exec.c    |    5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/testcases/kernel/security/filecaps/verify_caps_exec.c b/testcases/kernel/security/filecaps/verify_caps_exec.c
> index c3f65a9..605f0f6 100644
> --- a/testcases/kernel/security/filecaps/verify_caps_exec.c
> +++ b/testcases/kernel/security/filecaps/verify_caps_exec.c
> @@ -182,9 +182,10 @@ int fork_drop_and_exec(int keepperms, cap_t expected_caps)
> 			tst_resm(TINFO, "got a bad seqno (c=%d, s=%d, seqno=%d)",
> 				c, s, seqno);
> 		}
> -		p = index(buf, '.')+1;
> -		if (p==(char *)1)
> +		p = index(buf, '.');
> +		if (!p)
> 			tst_brkm(TFAIL, tst_exit, "got a bad message from print_caps\n");
> +		p += 1;
> 		actual_caps = cap_from_text(p);
> 		if (cap_compare(actual_caps, expected_caps) != 0) {
> 			capstxt = cap_to_text(expected_caps, NULL);

Looks good! If that's the complete diff, then Acked-by: Garrett Cooper <yan...@gm...>