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

[Tyler Hicks]

On 2014-02-05 12:10:04, Steve Beattie wrote:
> 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>

Good suggestion. I made this change and pushed. Thanks for the review!

Tyler
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20140205/7d34fc18/attachment.pgp>