[PATCH] tools, perf: Add asprintf replacement

[PATCH] tools, perf: Add asprintf replacement

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

asprintf corrupts memory on some older glibc versions.
Provide a replacement. This fixes various segfaults
with --branch-history on older Fedoras.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/Makefile.perf   |  1 +
 tools/perf/util/asprintf.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)
 create mode 100644 tools/perf/util/asprintf.c

diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index 1f7ec48..5174fb9 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -373,6 +373,7 @@ LIB_OBJS += $(OUTPUT)util/vdso.o
 LIB_OBJS += $(OUTPUT)util/stat.o
 LIB_OBJS += $(OUTPUT)util/record.o
 LIB_OBJS += $(OUTPUT)util/srcline.o
+LIB_OBJS += $(OUTPUT)util/asprintf.o
 LIB_OBJS += $(OUTPUT)util/data.o
 
 LIB_OBJS += $(OUTPUT)ui/setup.o
diff --git a/tools/perf/util/asprintf.c b/tools/perf/util/asprintf.c
new file mode 100644
index 0000000..9aafaca
--- /dev/null
+++ b/tools/perf/util/asprintf.c
@@ -0,0 +1,28 @@
+/* Replacement for asprintf as it's buggy in older glibc versions */
+#include <stdio.h>
+#include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
+
+int vasprintf(char **str, const char *fmt, va_list ap)
+{
+	char buf[1024];
+	int len = vsnprintf(buf, sizeof buf, fmt, ap);
+
+	*str = malloc(len + 1);
+	if (!*str)
+		return -1;
+	strcpy(*str, buf);
+	return len;
+}
+
+int asprintf(char **str, const char *fmt, ...)
+{
+	va_list ap;
+	int ret;
+
+	va_start(ap, fmt);
+	ret = vasprintf(str, fmt, ap);
+	va_end(ap);
+	return ret;
+}
-- 
1.8.5.3

Re: [PATCH] tools, perf: Add asprintf replacement

[Arnaldo Carvalho de Melo]

Em Mon, Mar 10, 2014 at 11:43:24PM -0700, Andi Kleen escreveu:
> From: Andi Kleen <ak@linux.intel.com>
> 
> asprintf corrupts memory on some older glibc versions.
> Provide a replacement. This fixes various segfaults
> with --branch-history on older Fedoras.

Humm, this unconditionally replaces it with an alternative that limits
the buffer to a fixed size :-\

Do you recall at least one of those old glibc version/release number?

A reproducer? So that I can try to reproduce it here and try to polish
this a bit more...

- Arnaldo
 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  tools/perf/Makefile.perf   |  1 +
>  tools/perf/util/asprintf.c | 28 ++++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
>  create mode 100644 tools/perf/util/asprintf.c
> 
> diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
> index 1f7ec48..5174fb9 100644
> --- a/tools/perf/Makefile.perf
> +++ b/tools/perf/Makefile.perf
> @@ -373,6 +373,7 @@ LIB_OBJS += $(OUTPUT)util/vdso.o
>  LIB_OBJS += $(OUTPUT)util/stat.o
>  LIB_OBJS += $(OUTPUT)util/record.o
>  LIB_OBJS += $(OUTPUT)util/srcline.o
> +LIB_OBJS += $(OUTPUT)util/asprintf.o
>  LIB_OBJS += $(OUTPUT)util/data.o
>  
>  LIB_OBJS += $(OUTPUT)ui/setup.o
> diff --git a/tools/perf/util/asprintf.c b/tools/perf/util/asprintf.c
> new file mode 100644
> index 0000000..9aafaca
> --- /dev/null
> +++ b/tools/perf/util/asprintf.c
> @@ -0,0 +1,28 @@
> +/* Replacement for asprintf as it's buggy in older glibc versions */
> +#include <stdio.h>
> +#include <stdarg.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +int vasprintf(char **str, const char *fmt, va_list ap)
> +{
> +	char buf[1024];
> +	int len = vsnprintf(buf, sizeof buf, fmt, ap);
> +
> +	*str = malloc(len + 1);
> +	if (!*str)
> +		return -1;
> +	strcpy(*str, buf);
> +	return len;
> +}
> +
> +int asprintf(char **str, const char *fmt, ...)
> +{
> +	va_list ap;
> +	int ret;
> +
> +	va_start(ap, fmt);
> +	ret = vasprintf(str, fmt, ap);
> +	va_end(ap);
> +	return ret;
> +}
> -- 
> 1.8.5.3

Re: [PATCH] tools, perf: Add asprintf replacement

[Andi Kleen]

> Humm, this unconditionally replaces it with an alternative that limits
> the buffer to a fixed size :-\

Better than corrupting memory.

I guess you could use two passes to avoid the limit, but it would surprise me
if anything in perf needs more than 1K of printf. One issue 
with doing two passes is that I wasn't sure the snprintf return
value would work properly on all libcs (e.g. the weirdo one Android uses)

> 
> Do you recall at least one of those old glibc version/release number?

glibc-2.13-2.x86_64 (FC14)

> 
> A reproducer? So that I can try to reproduce it here and try to polish
> this a bit more...

I saw it with perf report --branch-history in TUI mode and then pressing
e. But even running valgrind in stdio mode showed some corruption.
Without the patch also using some of the --call-graph options segfaulted.

-Andi

Re: [PATCH] tools, perf: Add asprintf replacement

[Arnaldo Carvalho de Melo]

Em Tue, Mar 18, 2014 at 08:05:33AM -0700, Andi Kleen escreveu:
> > Humm, this unconditionally replaces it with an alternative that limits
> > the buffer to a fixed size :-\
> 
> Better than corrupting memory.

Yes, it is better than corrupting memory, use the less ugly, good point.
 
> I guess you could use two passes to avoid the limit, but it would surprise me
> if anything in perf needs more than 1K of printf. One issue 
> with doing two passes is that I wasn't sure the snprintf return

The return of snprintf is crazy, that is why we use scnprintf.

> value would work properly on all libcs (e.g. the weirdo one Android uses)
> 
> > 
> > Do you recall at least one of those old glibc version/release number?
 
> glibc-2.13-2.x86_64 (FC14)

> > A reproducer? So that I can try to reproduce it here and try to polish
> > this a bit more...
> 
> I saw it with perf report --branch-history in TUI mode and then pressing
> e. But even running valgrind in stdio mode showed some corruption.
> Without the patch also using some of the --call-graph options segfaulted.

Thanks for the data points,

- Arnaldo