From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x830.google.com (mail-qt1-x830.google.com [IPv6:2607:f8b0:4864:20::830]) by sourceware.org (Postfix) with ESMTPS id 1D5F4385840A for ; Mon, 15 Aug 2022 19:20:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1D5F4385840A Received: by mail-qt1-x830.google.com with SMTP id h21so6200696qta.3 for ; Mon, 15 Aug 2022 12:20:12 -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=aMUV4QCutpDV29xNLqUi62l4hoKQjxU6fiH4oGHtbV0=; b=SF1O3DBi1qoCMQTk/W4fzdAON29NIHbMj30cM6hxRt7wJXmMWGHvXp6mTJpTJ/YHPV KeDOcbKhPidf9PDXYpPbRcFk20E6paWVD9NjTKBIb/hf9Rh2OaxBG7nowq9n225c8feg 23eTj9G8pmHdMlX23NhC6n4ZTAc6660H6qzbreEPXgXtBR4qBetyju/IL2C2Hq8T3MpY d22sYjuKetnwINKmjBZ9Bbaj7r0H0ORNSRiohkqF7PiSDLtU0tlzgHHyMukt8+DlnpzJ P2AxJ6GLcEn9H1m+zu3fGOhhLrhq8g4iuAdBk14o17WuV3YacNwdi6Rjmg6d2knqLEel Tp8w== X-Gm-Message-State: ACgBeo2GAstOOsi8gCIdj8gsTpzbqPGcP6lnNjJkkM8UuCyEeB5CY/Nf 0YTgo1Ucsb38k81J71BmKFgJbKwi5Udd5px0babkvX2ZJDU= X-Google-Smtp-Source: AA6agR79gMlYOUX1GJm3dvVnNQyCmQ6nyg/7QpB7Inj4G8o49x4hV3AkPqAnTiyy3EdlIDIArLMIV2Hj9BpTSA1XUNU= X-Received: by 2002:a05:622a:44c:b0:343:7b8e:2cb with SMTP id o12-20020a05622a044c00b003437b8e02cbmr12436969qtx.617.1660591211311; Mon, 15 Aug 2022 12:20:11 -0700 (PDT) MIME-Version: 1.0 References: <86a8594d-d3f5-a895-75c3-6a751c398b4e@suse.com> In-Reply-To: <86a8594d-d3f5-a895-75c3-6a751c398b4e@suse.com> From: "H.J. Lu" Date: Mon, 15 Aug 2022 12:19:35 -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: multipart/mixed; boundary="000000000000456b5305e64c85fc" X-Spam-Status: No, score=-3025.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, 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: Mon, 15 Aug 2022 19:20:19 -0000 --000000000000456b5305e64c85fc Content-Type: text/plain; charset="UTF-8" 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) -- H.J. --000000000000456b5305e64c85fc Content-Type: text/x-patch; charset="US-ASCII"; name="p.diff" Content-Disposition: attachment; filename="p.diff" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_l6v51vps0 ZGlmZiAtLWdpdCBhL29wY29kZXMvaTM4Ni1kaXMuYyBiL29wY29kZXMvaTM4Ni1kaXMuYwppbmRl eCAxNzYxZGY1ODNkYS4uYzExNjZhNDQ0NmMgMTAwNjQ0Ci0tLSBhL29wY29kZXMvaTM4Ni1kaXMu YworKysgYi9vcGNvZGVzL2kzODYtZGlzLmMKQEAgLTExOSw2ICsxMTksOSBAQCBzdGF0aWMgdm9p ZCBEaXN0aW5jdERlc3RfRml4dXAgKGluc3RyX2luZm8gKiwgaW50LCBpbnQpOwogICAgYnVmZmVy cy4gIFNlZSBvYXBwZW5kX2luc2VydF9zdHlsZSBmb3IgbW9yZSBkZXRhaWxzLiAgKi8KICNkZWZp bmUgU1RZTEVfTUFSS0VSX0NIQVIgJ1wwMDInCiAKKy8qIFRoZSBtYXhpbXVtIG9wZXJhbmQgYnVm ZmVyIHNpemUuICAqLworI2RlZmluZSBNQVhfT1BFUkFORF9CVUZGRVJfU0laRSAxMjgKKwogc3Ry dWN0IGRpc19wcml2YXRlIHsKICAgLyogUG9pbnRzIHRvIGZpcnN0IGJ5dGUgbm90IGZldGNoZWQu ICAqLwogICBiZmRfYnl0ZSAqbWF4X2ZldGNoZWQ7CkBAIC0xNjUsNyArMTY4LDcgQEAgc3RydWN0 IGluc3RyX2luZm8KICAgICAgY3VycmVudCBpbnN0cnVjdGlvbi4gICovCiAgIGludCBldmV4X3Vz ZWQ7CiAKLSAgY2hhciBvYnVmWzEwMF07CisgIGNoYXIgb2J1ZltNQVhfT1BFUkFORF9CVUZGRVJf U0laRV07CiAgIGNoYXIgKm9idWZwOwogICBjaGFyICptbmVtb25pY2VuZHA7CiAgIHVuc2lnbmVk IGNoYXIgKnN0YXJ0X2NvZGVwOwpAQCAtOTI3NSw3ICs5Mjc4LDcgQEAgaTM4Nl9kaXNfcHJpbnRm IChpbnN0cl9pbmZvICppbnMsIGVudW0gZGlzYXNzZW1ibGVyX3N0eWxlIHN0eWxlLAogICB2YV9s aXN0IGFwOwogICBlbnVtIGRpc2Fzc2VtYmxlcl9zdHlsZSBjdXJyX3N0eWxlID0gc3R5bGU7CiAg IGNoYXIgKnN0YXJ0LCAqY3VycjsKLSAgY2hhciBzdGFnaW5nX2FyZWFbMTAwXTsKKyAgY2hhciBz dGFnaW5nX2FyZWFbTUFYX09QRVJBTkRfQlVGRkVSX1NJWkVdOwogICBpbnQgcmVzOwogCiAgIHZh X3N0YXJ0IChhcCwgZm10KTsKQEAgLTkzNzcsNyArOTM4MCw3IEBAIHByaW50X2luc24gKGJmZF92 bWEgcGMsIGRpc2Fzc2VtYmxlX2luZm8gKmluZm8sIGludCBpbnRlbF9zeW50YXgpCiAgICAgLmxh c3Rfc2VnX3ByZWZpeCA9IC0xLAogICAgIC5md2FpdF9wcmVmaXggPSAtMSwKICAgfTsKLSAgY2hh ciBvcF9vdXRbTUFYX09QRVJBTkRTXVsxMDBdOworICBjaGFyIG9wX291dFtNQVhfT1BFUkFORFNd W01BWF9PUEVSQU5EX0JVRkZFUl9TSVpFXTsKIAogICBwcml2Lm9yaWdfc2l6ZWZsYWcgPSBBRkxB RyB8IERGTEFHOwogICBpZiAoKGluZm8tPm1hY2ggJiBiZmRfbWFjaF9pMzg2X2kzODYpICE9IDAp Cg== --000000000000456b5305e64c85fc--