[v1] perf probe: Fix uninitialized variable

[PATCH] perf probe: Fix uninitialized variable

Posted by James Clark 2 weeks, 3 days ago

Since the linked fixes: commit, err is returned uninitialized due to the
removal of "return 0". Initialize err to fix it, and rename err to out
to avoid confusion because buf is still supposed to be freed in non
error cases.

This fixes the following intermittent test failure on release builds:

 $ perf test "testsuite_probe"
 ...
 -- [ FAIL ] -- perf_probe :: test_invalid_options :: mutually exclusive options :: -L foo -V bar (output regexp parsing)
 Regexp not found: \"Error: switch .+ cannot be used with switch .+\"
 ...

Fixes: 080e47b2a237 ("perf probe: Introduce quotation marks support")
Signed-off-by: James Clark <james.clark@linaro.org>
---
 tools/perf/util/probe-event.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 6d51a4c98ad7..35af6570cf9b 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -1370,7 +1370,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
 {
 	char *buf = strdup(arg);
 	char *p;
-	int err;
+	int err = 0;
 
 	if (!buf)
 		return -ENOMEM;
@@ -1383,20 +1383,20 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
 		if (p == buf) {
 			semantic_error("No file/function name in '%s'.\n", p);
 			err = -EINVAL;
-			goto err;
+			goto out;
 		}
 		*(p++) = '\0';
 
 		err = parse_line_num(&p, &lr->start, "start line");
 		if (err)
-			goto err;
+			goto out;
 
 		if (*p == '+' || *p == '-') {
 			const char c = *(p++);
 
 			err = parse_line_num(&p, &lr->end, "end line");
 			if (err)
-				goto err;
+				goto out;
 
 			if (c == '+') {
 				lr->end += lr->start;
@@ -1416,11 +1416,11 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
 		if (lr->start > lr->end) {
 			semantic_error("Start line must be smaller"
 				       " than end line.\n");
-			goto err;
+			goto out;
 		}
 		if (*p != '\0') {
 			semantic_error("Tailing with invalid str '%s'.\n", p);
-			goto err;
+			goto out;
 		}
 	}
 
@@ -1431,7 +1431,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
 			lr->file = strdup_esq(p);
 			if (lr->file == NULL) {
 				err = -ENOMEM;
-				goto err;
+				goto out;
 			}
 		}
 		if (*buf != '\0')
@@ -1439,7 +1439,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
 		if (!lr->function && !lr->file) {
 			semantic_error("Only '@*' is not allowed.\n");
 			err = -EINVAL;
-			goto err;
+			goto out;
 		}
 	} else if (strpbrk_esq(buf, "/."))
 		lr->file = strdup_esq(buf);
@@ -1448,10 +1448,10 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
 	else {	/* Invalid name */
 		semantic_error("'%s' is not a valid function name.\n", buf);
 		err = -EINVAL;
-		goto err;
+		goto out;
 	}
 
-err:
+out:
 	free(buf);
 	return err;
 }
-- 
2.34.1

Re: [PATCH] perf probe: Fix uninitialized variable

Posted by Masami Hiramatsu (Google) 2 weeks, 1 day ago

Hi,

On Mon,  9 Dec 2024 17:12:21 +0000
James Clark <james.clark@linaro.org> wrote:

> Since the linked fixes: commit, err is returned uninitialized due to the
> removal of "return 0". Initialize err to fix it, and rename err to out
> to avoid confusion because buf is still supposed to be freed in non
> error cases.
> 
> This fixes the following intermittent test failure on release builds:
> 
>  $ perf test "testsuite_probe"
>  ...
>  -- [ FAIL ] -- perf_probe :: test_invalid_options :: mutually exclusive options :: -L foo -V bar (output regexp parsing)
>  Regexp not found: \"Error: switch .+ cannot be used with switch .+\"
>  ...
> 
> Fixes: 080e47b2a237 ("perf probe: Introduce quotation marks support")
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  tools/perf/util/probe-event.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 6d51a4c98ad7..35af6570cf9b 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1370,7 +1370,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  {
>  	char *buf = strdup(arg);
>  	char *p;
> -	int err;
> +	int err = 0;

I think only this is required, and others are just cleanups by renaming
err -> out (usually for-next).
But Arnaldo is OK to combine these changes, I'm OK too.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thank you,

>  
>  	if (!buf)
>  		return -ENOMEM;
> @@ -1383,20 +1383,20 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  		if (p == buf) {
>  			semantic_error("No file/function name in '%s'.\n", p);
>  			err = -EINVAL;
> -			goto err;
> +			goto out;
>  		}
>  		*(p++) = '\0';
>  
>  		err = parse_line_num(&p, &lr->start, "start line");
>  		if (err)
> -			goto err;
> +			goto out;
>  
>  		if (*p == '+' || *p == '-') {
>  			const char c = *(p++);
>  
>  			err = parse_line_num(&p, &lr->end, "end line");
>  			if (err)
> -				goto err;
> +				goto out;
>  
>  			if (c == '+') {
>  				lr->end += lr->start;
> @@ -1416,11 +1416,11 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  		if (lr->start > lr->end) {
>  			semantic_error("Start line must be smaller"
>  				       " than end line.\n");
> -			goto err;
> +			goto out;
>  		}
>  		if (*p != '\0') {
>  			semantic_error("Tailing with invalid str '%s'.\n", p);
> -			goto err;
> +			goto out;
>  		}
>  	}
>  
> @@ -1431,7 +1431,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  			lr->file = strdup_esq(p);
>  			if (lr->file == NULL) {
>  				err = -ENOMEM;
> -				goto err;
> +				goto out;
>  			}
>  		}
>  		if (*buf != '\0')
> @@ -1439,7 +1439,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  		if (!lr->function && !lr->file) {
>  			semantic_error("Only '@*' is not allowed.\n");
>  			err = -EINVAL;
> -			goto err;
> +			goto out;
>  		}
>  	} else if (strpbrk_esq(buf, "/."))
>  		lr->file = strdup_esq(buf);
> @@ -1448,10 +1448,10 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  	else {	/* Invalid name */
>  		semantic_error("'%s' is not a valid function name.\n", buf);
>  		err = -EINVAL;
> -		goto err;
> +		goto out;
>  	}
>  
> -err:
> +out:
>  	free(buf);
>  	return err;
>  }
> -- 
> 2.34.1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] perf probe: Fix uninitialized variable

Posted by Arnaldo Carvalho de Melo 2 weeks, 2 days ago

On Mon, Dec 09, 2024 at 05:12:21PM +0000, James Clark wrote:
> Since the linked fixes: commit, err is returned uninitialized due to the
> removal of "return 0". Initialize err to fix it, and rename err to out
> to avoid confusion because buf is still supposed to be freed in non
> error cases.
> 
> This fixes the following intermittent test failure on release builds:
> 
>  $ perf test "testsuite_probe"
>  ...
>  -- [ FAIL ] -- perf_probe :: test_invalid_options :: mutually exclusive options :: -L foo -V bar (output regexp parsing)
>  Regexp not found: \"Error: switch .+ cannot be used with switch .+\"
>  ...

Namhyung, I think this should go via perf-tools,

Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo
 
> Fixes: 080e47b2a237 ("perf probe: Introduce quotation marks support")
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  tools/perf/util/probe-event.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 6d51a4c98ad7..35af6570cf9b 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1370,7 +1370,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  {
>  	char *buf = strdup(arg);
>  	char *p;
> -	int err;
> +	int err = 0;
>  
>  	if (!buf)
>  		return -ENOMEM;
> @@ -1383,20 +1383,20 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  		if (p == buf) {
>  			semantic_error("No file/function name in '%s'.\n", p);
>  			err = -EINVAL;
> -			goto err;
> +			goto out;
>  		}
>  		*(p++) = '\0';
>  
>  		err = parse_line_num(&p, &lr->start, "start line");
>  		if (err)
> -			goto err;
> +			goto out;
>  
>  		if (*p == '+' || *p == '-') {
>  			const char c = *(p++);
>  
>  			err = parse_line_num(&p, &lr->end, "end line");
>  			if (err)
> -				goto err;
> +				goto out;
>  
>  			if (c == '+') {
>  				lr->end += lr->start;
> @@ -1416,11 +1416,11 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  		if (lr->start > lr->end) {
>  			semantic_error("Start line must be smaller"
>  				       " than end line.\n");
> -			goto err;
> +			goto out;
>  		}
>  		if (*p != '\0') {
>  			semantic_error("Tailing with invalid str '%s'.\n", p);
> -			goto err;
> +			goto out;
>  		}
>  	}
>  
> @@ -1431,7 +1431,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  			lr->file = strdup_esq(p);
>  			if (lr->file == NULL) {
>  				err = -ENOMEM;
> -				goto err;
> +				goto out;
>  			}
>  		}
>  		if (*buf != '\0')
> @@ -1439,7 +1439,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  		if (!lr->function && !lr->file) {
>  			semantic_error("Only '@*' is not allowed.\n");
>  			err = -EINVAL;
> -			goto err;
> +			goto out;
>  		}
>  	} else if (strpbrk_esq(buf, "/."))
>  		lr->file = strdup_esq(buf);
> @@ -1448,10 +1448,10 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  	else {	/* Invalid name */
>  		semantic_error("'%s' is not a valid function name.\n", buf);
>  		err = -EINVAL;
> -		goto err;
> +		goto out;
>  	}
>  
> -err:
> +out:
>  	free(buf);
>  	return err;
>  }
> -- 
> 2.34.1
>

Re: [PATCH] perf probe: Fix uninitialized variable

Posted by Namhyung Kim 2 weeks, 2 days ago

On Mon, Dec 09, 2024 at 05:12:21PM +0000, James Clark wrote:
> Since the linked fixes: commit, err is returned uninitialized due to the
> removal of "return 0". Initialize err to fix it, and rename err to out
> to avoid confusion because buf is still supposed to be freed in non
> error cases.
> 
> This fixes the following intermittent test failure on release builds:
> 
>  $ perf test "testsuite_probe"
>  ...
>  -- [ FAIL ] -- perf_probe :: test_invalid_options :: mutually exclusive options :: -L foo -V bar (output regexp parsing)
>  Regexp not found: \"Error: switch .+ cannot be used with switch .+\"
>  ...

Tested-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

> 
> Fixes: 080e47b2a237 ("perf probe: Introduce quotation marks support")
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  tools/perf/util/probe-event.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 6d51a4c98ad7..35af6570cf9b 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1370,7 +1370,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  {
>  	char *buf = strdup(arg);
>  	char *p;
> -	int err;
> +	int err = 0;
>  
>  	if (!buf)
>  		return -ENOMEM;
> @@ -1383,20 +1383,20 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  		if (p == buf) {
>  			semantic_error("No file/function name in '%s'.\n", p);
>  			err = -EINVAL;
> -			goto err;
> +			goto out;
>  		}
>  		*(p++) = '\0';
>  
>  		err = parse_line_num(&p, &lr->start, "start line");
>  		if (err)
> -			goto err;
> +			goto out;
>  
>  		if (*p == '+' || *p == '-') {
>  			const char c = *(p++);
>  
>  			err = parse_line_num(&p, &lr->end, "end line");
>  			if (err)
> -				goto err;
> +				goto out;
>  
>  			if (c == '+') {
>  				lr->end += lr->start;
> @@ -1416,11 +1416,11 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  		if (lr->start > lr->end) {
>  			semantic_error("Start line must be smaller"
>  				       " than end line.\n");
> -			goto err;
> +			goto out;
>  		}
>  		if (*p != '\0') {
>  			semantic_error("Tailing with invalid str '%s'.\n", p);
> -			goto err;
> +			goto out;
>  		}
>  	}
>  
> @@ -1431,7 +1431,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  			lr->file = strdup_esq(p);
>  			if (lr->file == NULL) {
>  				err = -ENOMEM;
> -				goto err;
> +				goto out;
>  			}
>  		}
>  		if (*buf != '\0')
> @@ -1439,7 +1439,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  		if (!lr->function && !lr->file) {
>  			semantic_error("Only '@*' is not allowed.\n");
>  			err = -EINVAL;
> -			goto err;
> +			goto out;
>  		}
>  	} else if (strpbrk_esq(buf, "/."))
>  		lr->file = strdup_esq(buf);
> @@ -1448,10 +1448,10 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  	else {	/* Invalid name */
>  		semantic_error("'%s' is not a valid function name.\n", buf);
>  		err = -EINVAL;
> -		goto err;
> +		goto out;
>  	}
>  
> -err:
> +out:
>  	free(buf);
>  	return err;
>  }
> -- 
> 2.34.1
>