[apparmor] [PATCH] parser: Quiet search dir valgrind warning and remove suppression

[Steve Beattie]

On Wed, Feb 05, 2014 at 02:51:30PM -0500, Tyler Hicks wrote:
> When passing an include directory on the command line to
> apparmor_parser, valgrind emits a warning:
> 
>  Invalid read of size 4
>     at 0x404DA6: add_search_dir(char const*) (parser_include.c:152)
>     by 0x40BB37: process_arg(int, char*) (parser_main.c:457)
>     by 0x403D43: main (parser_main.c:590)
>   Address 0x572207c is 28 bytes inside a block of size 29 alloc'd
>     at 0x4C2A420: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>     by 0x53E31C9: strdup (strdup.c:42)
>     by 0x404D94: add_search_dir(char const*) (parser_include.c:145)
>     by 0x40BB37: process_arg(int, char*) (parser_main.c:457)
>     by 0x403D43: main (parser_main.c:590)
> 
> This patch quiets the warning by removing strlen() calls on the t char
> array. Instead, it only calls strlen() on the dir char array. t is a
> dupe of dir and strlen(dir) does not trigger the valgrind warning.
> 
> Additionally, this patch adds a bit of defensive programming to the
> while loop to ensure that index into the t array is never negative.
> 
> Finally, the valgrind suppression is removed from valgrind_simple.py.
> 
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>



> ---
>  parser/parser_include.c       | 12 +++++++++---
>  parser/tst/valgrind_simple.py |  8 --------
>  2 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/parser/parser_include.c b/parser/parser_include.c
> index 0ad865d..3947f29 100644
> --- a/parser/parser_include.c
> +++ b/parser/parser_include.c
> @@ -133,13 +133,19 @@ void set_base_dir(char *dir)
>  int add_search_dir(const char *dir)
>  {
>  	char *t;
> +	size_t len;
> +
>  	if (npath >= MAX_PATH) {
>  		PERROR(_("Error: Could not add directory %s to search path.\n"),
>  		       dir);
>  		return 0;
>  	}
>  
> -	if (!dir || strlen(dir) <= 0)
> +	if (!dir)
> +		return 1;
> +
> +	len = strlen(dir);
> +	if (len == 0)
>  		return 1;
>  
>  	t = strdup(dir);
> @@ -149,8 +155,8 @@ int add_search_dir(const char *dir)
>  	}
>  
>  	/*strip trailing /'s */
> -	while (t[strlen(t) - 1] == '/')
> -		t[strlen(t) - 1] = 0;
> +	while (len > 0 && t[--len] == '/')
> +		t[len] = 0;

Can you convert the assignment from 0 to '\0'? With that change,
Acked-by: Steve Beattie <steve at nxnw.org>

Thanks!

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20140205/aab41de2/attachment.pgp>