From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x831.google.com (mail-qt1-x831.google.com [IPv6:2607:f8b0:4864:20::831]) by sourceware.org (Postfix) with ESMTPS id 270083858CDA for ; Tue, 16 Aug 2022 15:15:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 270083858CDA Received: by mail-qt1-x831.google.com with SMTP id cr9so8324763qtb.13 for ; Tue, 16 Aug 2022 08:15:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc; bh=dpElJACh7pe6ZobiDE2h+Tca8U+BbIJwDmWQMxj+rP4=; b=3XCc5ExJnEufWJRg5xBYxYoP/4qVqNHueMACfgWF+VzEVz76MP5Vb+s4dfHOg75q65 yQRnmf5q5HEukTh66ZjeOJctxLhbAkLX/YBJng05BJE3fvaoInuL9fWf+FmRZHkf5yd8 NHprDBmCIQAsDHatHGqFMgG+Xzfurd5xcUb3oDpNSh4iFI0r8F6LKQKE8ZPzzrjw6PiZ o1sOixKAYDn8whaaZM2GudxkzSamrtwl5k32r56DOKrh3tc5Mpvvduty4by+3g2gwArD w2zpit1BrL8nEayWLdmsz7WveZSXO0JIk9zULDjU72/zsL5S7YcR0NxQLNonMpFNpm/x jAKg== X-Gm-Message-State: ACgBeo0efuD63EnmJKrSl21fP+qtB/NMRNgpXg7sv+6MvMT2JDB8qT1b 2Gd9yWk0C1Lo60WheTHuW9sUJMr4la82lDjoyL5Wq5XVSzg= X-Google-Smtp-Source: AA6agR6lZ78dUhIQVWhcDqybL/x7L27kdBdXzZpkeKRaYSCCTR3rnAdY9bAzG+Jni0EKSDHEYeBHh5isB1o3WMYTCPw= X-Received: by 2002:a05:622a:44c:b0:343:7b8e:2cb with SMTP id o12-20020a05622a044c00b003437b8e02cbmr15659559qtx.617.1660662919321; Tue, 16 Aug 2022 08:15:19 -0700 (PDT) MIME-Version: 1.0 References: <86a8594d-d3f5-a895-75c3-6a751c398b4e@suse.com> <02a83431-ad0a-be06-f7bc-7757f80faa7c@suse.com> In-Reply-To: <02a83431-ad0a-be06-f7bc-7757f80faa7c@suse.com> From: "H.J. Lu" Date: Tue, 16 Aug 2022 08:14:43 -0700 Message-ID: Subject: Re: [PATCH v2] x86: avoid i386_dis_printf()'s staging area for potentially long strings To: Jan Beulich Cc: Binutils , Alan Modra Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3018.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: binutils@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Aug 2022 15:15:22 -0000 On Mon, Aug 15, 2022 at 11:06 PM Jan Beulich wrote: > > On 15.08.2022 21:19, H.J. Lu wrote: > > On Mon, Aug 15, 2022 at 8:38 AM Jan Beulich wrote: > >> > >> PR binutils/29483 > >> > >> When print_insn() processes op_txt[], it may pass strings into > >> i386_dis_printf() which staging_area[] cannot fit; this was observed for > >> an invalid form of VPSCATTERDD (both broadcast and zeroing-masking bits > >> set). Rather than arbitrarily enlarging that local array, avoid its use > >> altogether when the format string is simply "%s". This merely requires > >> two local variables to have their type constified. > >> > >> While limiting the scope of "res" it became apparent that > >> - no caller cares about the function's return value, > >> - the comment about the return value was wrong, > >> - a particular positive return value would have been meaningless to the > >> caller. > >> Therefore convert the function to return "void" at the same time. > >> --- > >> An alternative to the special casing would be to introduce something > >> like i386_dis_puts(), then to be used by all call sites which currently > >> pass "%s" or format strings without any format characters at all (plus, > >> of course, i386_dis_printf() itself). > >> --- > >> v2: Add testcase. > >> > >> --- a/gas/testsuite/gas/i386/i386.exp > >> +++ b/gas/testsuite/gas/i386/i386.exp > >> @@ -1349,6 +1349,7 @@ if [gas_64_check] then { > >> run_dump_test ehinterp > >> } > >> run_dump_test pr27198 > >> + run_dump_test pr29483 > >> > >> set ASFLAGS "$old_ASFLAGS --64" > >> > >> --- /dev/null > >> +++ b/gas/testsuite/gas/i386/pr29483.d > >> @@ -0,0 +1,11 @@ > >> +#objdump: -dw > >> +#name: x86-64 PR binutils/29483 > >> + > >> +.*: +file format .* > >> + > >> +Disassembly of section .text: > >> + > >> +0+ : > >> +[ ]*[a-f0-9]+: 65 62 62 7d 97 a0 94 ff 20 20 20 ae vpscatterdd .* > >> + > >> +0x65,0x62,0x62,0x7d,0x97,0xa0,0x94,0xff,0x20,0x20,0x20,0xae > >> --- /dev/null > >> +++ b/gas/testsuite/gas/i386/pr29483.s > >> @@ -0,0 +1,5 @@ > >> + .text > >> +pr29483: > >> + # This (VPSCATTERDD with EVEX.br and EVEX.z invalidly set) should not > >> + # crash the disassembler. > >> + .byte 0x65,0x62,0x62,0x7d,0x97,0xa0,0x94,0xff,0x20,0x20,0x20,0xae > >> --- a/opcodes/i386-dis.c > >> +++ b/opcodes/i386-dis.c > >> @@ -9264,31 +9264,40 @@ oappend_register (instr_info *ins, const > >> STYLE is the default style to use in the fprintf_styled_func calls, > >> however, FMT might include embedded style markers (see oappend_style), > >> these embedded markers are not printed, but instead change the style > >> - used in the next fprintf_styled_func call. > >> + used in the next fprintf_styled_func call. */ > >> > >> - Return non-zero to indicate the print call was a success. */ > >> - > >> -static int ATTRIBUTE_PRINTF_3 > >> +static void ATTRIBUTE_PRINTF_3 > >> i386_dis_printf (instr_info *ins, enum disassembler_style style, > >> const char *fmt, ...) > >> { > >> va_list ap; > >> enum disassembler_style curr_style = style; > >> - char *start, *curr; > >> + const char *start, *curr; > >> char staging_area[100]; > >> - int res; > >> > >> va_start (ap, fmt); > >> - res = vsnprintf (staging_area, sizeof (staging_area), fmt, ap); > >> - va_end (ap); > >> + /* In particular print_insn()'s processing of op_txt[] can hand rather long > >> + strings here. Bypass vsnprintf() in such cases to avoid capacity issues > >> + with the staging area. */ > >> + if (strcmp (fmt, "%s")) > >> + { > >> + int res = vsnprintf (staging_area, sizeof (staging_area), fmt, ap); > >> > >> - if (res < 0) > >> - return res; > >> + va_end (ap); > >> > >> - if ((size_t) res >= sizeof (staging_area)) > >> - abort (); > >> + if (res < 0) > >> + return; > >> > >> - start = curr = staging_area; > >> + if ((size_t) res >= sizeof (staging_area)) > >> + abort (); > >> + > >> + start = curr = staging_area; > >> + } > >> + else > >> + { > >> + start = curr = va_arg (ap, const char *); > >> + va_end (ap); > >> + } > >> > >> do > >> { > >> @@ -9303,10 +9312,7 @@ i386_dis_printf (instr_info *ins, enum d > >> curr_style, > >> "%.*s", len, start); > >> if (n < 0) > >> - { > >> - res = n; > >> - break; > >> - } > >> + break; > >> > >> if (*curr == '\0') > >> break; > >> @@ -9340,8 +9346,6 @@ i386_dis_printf (instr_info *ins, enum d > >> ++curr; > >> } > >> while (true); > >> - > >> - return res; > >> } > >> > >> static int > > > > I prefer something like this with the disassembler output: > > > > 0: 65 62 62 7d 97 a0 94 ff 20 20 20 ae vpscatterdd > > %xmm26,%gs:-0x51dfdfe0(%rdi,%xmm23,8){bad}{%k7}{z}/(bad) > > But the precise representation of the "badness" isn't relevant to this > testcase. All we care about is that there's no crash (and perhaps, see > below, no overrun of the other buffer, but I view this as a separate > aspect). I want to avoid the output like 65 62 62 7d 97 a0 94 ff 20 20 20 ae vpscatterdd ... 0x65,0x62,0x62,0x7d,0x97,0xa0,0x94,0xff,0x20,0x20,0x20,0xae > As to the attachment - I figure honoring op_out[]'s 2nd dimension is > a necessary thing. I don't think I agree with bumping the buffer size, > though: 128 is as arbitrary as 100. Or else there would want to be a > comment next to MAX_OPERAND_BUFFER_SIZE explaining how the value was > determined and hence making clear under what conditions it might need > further bumping. When styling was added, the original 100-byte buffer size wasn't increased. Increasing it by 28 bytes should cover styling characters. > Yet you're the maintainer - if you want to go with your fix, I won't > be able to stop you. > > Jan I will post a patch. -- H.J.