| Message ID | 20190704085856.17502-1-jolsa@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | tools bpftool: Fix json dump crash on powerpc | expand |
2019-07-04 10:58 UTC+0200 ~ Jiri Olsa <jolsa@kernel.org> > Michael reported crash with by bpf program in json mode on powerpc: > > # bpftool prog -p dump jited id 14 > [{ > "name": "0xd00000000a9aa760", > "insns": [{ > "pc": "0x0", > "operation": "nop", > "operands": [null > ] > },{ > "pc": "0x4", > "operation": "nop", > "operands": [null > ] > },{ > "pc": "0x8", > "operation": "mflr", > Segmentation fault (core dumped) > > The code is assuming char pointers in format, which is not always > true at least for powerpc. Fixing this by dumping the whole string > into buffer based on its format. > > Please note that libopcodes code does not check return values from > fprintf callback, so there's no point to return error in case of > allocation failure. > > Reported-by: Michael Petlan <mpetlan@redhat.com> > Signed-off-by: Jiri Olsa <jolsa@kernel.org> Looks good to me, thank you for the fix! Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
On Thu, 4 Jul 2019 10:58:56 +0200, Jiri Olsa wrote: > Michael reported crash with by bpf program in json mode on powerpc: > > # bpftool prog -p dump jited id 14 > [{ > "name": "0xd00000000a9aa760", > "insns": [{ > "pc": "0x0", > "operation": "nop", > "operands": [null > ] > },{ > "pc": "0x4", > "operation": "nop", > "operands": [null > ] > },{ > "pc": "0x8", > "operation": "mflr", > Segmentation fault (core dumped) > > The code is assuming char pointers in format, which is not always > true at least for powerpc. Fixing this by dumping the whole string > into buffer based on its format. > > Please note that libopcodes code does not check return values from > fprintf callback, so there's no point to return error in case of > allocation failure. Well, it doesn't check it today, it may perhaps do it in the future? Let's flip the question - since it doesn't check it today, why not propagate the error? :) We should stay close to how fprintf would behave, IMHO. Fixes: 107f041212c1 ("tools: bpftool: add JSON output for `bpftool prog dump jited *` command") > Reported-by: Michael Petlan <mpetlan@redhat.com> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
diff --git a/tools/bpf/bpftool/jit_disasm.c b/tools/bpf/bpftool/jit_disasm.c index 3ef3093560ba..05fa6dc970f8 100644 --- a/tools/bpf/bpftool/jit_disasm.c +++ b/tools/bpf/bpftool/jit_disasm.c @@ -11,6 +11,8 @@ * Licensed under the GNU General Public License, version 2.0 (GPLv2) */ +#define _GNU_SOURCE +#include <stdio.h> #include <stdarg.h> #include <stdint.h> #include <stdio.h> @@ -44,11 +46,13 @@ static int fprintf_json(void *out, const char *fmt, ...) char *s; va_start(ap, fmt); + if (vasprintf(&s, fmt, ap) < 0) + return 0; + va_end(ap); + if (!oper_count) { int i; - s = va_arg(ap, char *); - /* Strip trailing spaces */ i = strlen(s) - 1; while (s[i] == ' ') @@ -61,11 +65,10 @@ static int fprintf_json(void *out, const char *fmt, ...) } else if (!strcmp(fmt, ",")) { /* Skip */ } else { - s = va_arg(ap, char *); jsonw_string(json_wtr, s); oper_count++; } - va_end(ap); + free(s); return 0; }
Michael reported crash with by bpf program in json mode on powerpc: # bpftool prog -p dump jited id 14 [{ "name": "0xd00000000a9aa760", "insns": [{ "pc": "0x0", "operation": "nop", "operands": [null ] },{ "pc": "0x4", "operation": "nop", "operands": [null ] },{ "pc": "0x8", "operation": "mflr", Segmentation fault (core dumped) The code is assuming char pointers in format, which is not always true at least for powerpc. Fixing this by dumping the whole string into buffer based on its format. Please note that libopcodes code does not check return values from fprintf callback, so there's no point to return error in case of allocation failure. Reported-by: Michael Petlan <mpetlan@redhat.com> Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- tools/bpf/bpftool/jit_disasm.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)