[Ltrace-devel] [PATCH] Fix an uninitialized memory access in parse_lens

[Petr Machata]

Роман Донченко <dpb at corrigendum.ru> writes:

> own_lens is only initialized when lens != NULL.
> ---
>  read_config_file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/read_config_file.c b/read_config_file.c
> index 9d3ccd8..6e47955 100644
> --- a/read_config_file.c
> +++ b/read_config_file.c
> @@ -1009,7 +1009,7 @@ parse_lens(struct protolib *plib, struct locus *loc,
>  				  ownp, forwardp);
>  		if (info == NULL) {
>  		fail:
> -			if (own_lens && lens != NULL)
> +			if (lens != NULL && own_lens)
>  				lens_destroy(lens);
>  			return NULL;
>  		}

Oh the wonders of C.

I'm looking at name2lens--it only ever sets *own_lensp to 0.  Any
opinions about this?

diff --git a/read_config_file.c b/read_config_file.c
index 6e47955..c134bc2 100644
--- a/read_config_file.c
+++ b/read_config_file.c
@@ -1,6 +1,6 @@
 /*
  * This file is part of ltrace.
- * Copyright (C) 2011,2012,2013,2014 Petr Machata, Red Hat Inc.
+ * Copyright (C) 2011,2012,2013,2014,2015 Petr Machata, Red Hat Inc.
  * Copyright (C) 1998,1999,2003,2007,2008,2009 Juan Cespedes
  * Copyright (C) 2006 Ian Wienand
  * Copyright (C) 2006 Steve Fink
@@ -932,15 +932,12 @@ static struct named_lens {
 };
 
 static struct lens *
-name2lens(char **str, int *own_lensp)
+name2lens(char **str)
 {
 	size_t i;
 	for (i = 0; i < sizeof(lenses)/sizeof(*lenses); ++i)
-		if (try_parse_kwd(str, lenses[i].name) == 0) {
-			*own_lensp = 0;
+		if (try_parse_kwd(str, lenses[i].name) == 0)
 			return lenses[i].lens;
-		}
-
 	return NULL;
 }
 
@@ -983,8 +980,9 @@ parse_lens(struct protolib *plib, struct locus *loc,
 	   char **str, struct param **extra_param,
 	   size_t param_num, int *ownp, int *forwardp)
 {
-	int own_lens;
-	struct lens *lens = name2lens(str, &own_lens);
+	struct lens *lens = name2lens(str);
+	// N.B.: LENS is not owned, and should not be destroyed.
+
 	int has_args = 1;
 	struct arg_type_info *info;
 	if (lens != NULL) {
@@ -1007,12 +1005,8 @@ parse_lens(struct protolib *plib, struct locus *loc,
 		eat_spaces(str);
 		info = parse_type(plib, loc, str, extra_param, param_num,
 				  ownp, forwardp);
-		if (info == NULL) {
-		fail:
-			if (lens != NULL && own_lens)
-				lens_destroy(lens);
+		if (info == NULL)
 			return NULL;
-		}
 	}
 
 	if (lens != NULL && has_args) {
@@ -1023,11 +1017,11 @@ parse_lens(struct protolib *plib, struct locus *loc,
 	/* We can't modify shared types.  Make a copy if we have a
 	 * lens.  */
 	if (lens != NULL && unshare_type_info(loc, &info, ownp) < 0)
-		goto fail;
+		return NULL;
 
 	if (lens != NULL) {
 		info->lens = lens;
-		info->own_lens = own_lens;
+		info->own_lens = 0;
 	}
 
 	return info;

It's got a lot of -'s, which I generally like :)  But we sort of
hard-code the knowledge that lens is unowned, which makes things
possibly not obvious.

Thanks,
Petr