From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x331.google.com (mail-ot1-x331.google.com [IPv6:2607:f8b0:4864:20::331]) by sourceware.org (Postfix) with ESMTPS id 11E4D3858D20 for ; Mon, 12 Jun 2023 20:15:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 11E4D3858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-ot1-x331.google.com with SMTP id 46e09a7af769-6b2b7ca1c5eso3117858a34.0 for ; Mon, 12 Jun 2023 13:15:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1686600950; x=1689192950; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=PZkJ7Qz3SLWg34YpXT4MKtNVQteY4nV1TI6EgOMDjrk=; b=PY519luMtUE2nGKmAEuTPOYA9bwfAwfp89fITEbQERo88JJ/HH8wSjb7cROia7+D2o qtXYp61LhGuZRvnUOBV1d51z3GTYnSExsrgC895NOmUGzgxVw5nemTlnuSHzyOhLHLrp aItbtCnOba0DhrzPjoFFuK3KBNGtozt94FT8kGpkxFZMP7+OX5mIvdZrWvk4IRxIHeLO diL0jjuzUfj7BBpSKW2EprI+yWl2lTsFqKbYStxafkkiVOj1nLmMQxJKT3hpbDx/FmH7 4OzB/SxKd+FcKS3Ec7GGBocTt52YSKG19XbhMezz5QUH0EC0XXM1tiCaPr/OBNCvB6zk OUKg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686600950; x=1689192950; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=PZkJ7Qz3SLWg34YpXT4MKtNVQteY4nV1TI6EgOMDjrk=; b=PDC21IZvK/tndjms8Rl7jXwwiHtE1XESqkrbf+ETFbJAc/lpMCHnutqXgNUocpEWsR eFC4tiPMd6pUL88bphXCoYulQrRuXgBWmt48sLhK2j5BJ94Lez3x1eeYc7TJxMzzAXxh wMZ+WqYfgbmys+qaO1uwrXfeJR1dn957q9HNwZHnP2Y+/LV8dEODt4moB9pchl7o9W2y LF/i2rcwC6LZucbgFzrUJw0PTmblbtROvgM49p5GQkpUMDsgH+T+cfHKWtNXBn8wc7r9 P3St1Uhwgk2m83hbBRoT+QwAUGdKRHqjE6NBKDKn72M6fntWxxCQDDPuKAETU1BI4k4h yxAg== X-Gm-Message-State: AC+VfDw84eC51IuQ7tTOjbERYdP7lZ46robeP29qwHVvA4lXXPmcodiy LeCfICNexlQkd3UJW9SlhwYzwYNE75dTyFEw/sv8dw== X-Google-Smtp-Source: ACHHUZ7MeWNvhDkO/bQorBmoUekx3FR8RS+H9EcA8rkAOHfumOBICSonAm3VAQxHSlNM9xwhOg0Rkg== X-Received: by 2002:a05:6870:a4c6:b0:18e:2b7e:a844 with SMTP id k6-20020a056870a4c600b0018e2b7ea844mr6701090oal.21.1686600949707; Mon, 12 Jun 2023 13:15:49 -0700 (PDT) Received: from ?IPV6:2804:1b3:a7c2:8501:140e:99f1:7d92:1ee7? ([2804:1b3:a7c2:8501:140e:99f1:7d92:1ee7]) by smtp.gmail.com with ESMTPSA id y39-20020a056870b4a700b0019e59515a0bsm6344386oap.33.2023.06.12.13.15.48 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 12 Jun 2023 13:15:48 -0700 (PDT) Message-ID: <94b97bcd-c42e-5aad-abcc-ab857b1b4e32@linaro.org> Date: Mon, 12 Jun 2023 17:15:46 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [PATCH v3] vfprintf-internal: Get rid of alloca. Content-Language: en-US To: libc-alpha@sourceware.org, Joe Simmons-Talbott References: <20230522170627.239536-1-josimmon@redhat.com> From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: <20230522170627.239536-1-josimmon@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,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 List-Id: On 22/05/23 14:06, Joe Simmons-Talbott via Libc-alpha wrote: > Avoid potential stack overflow from unbounded alloca. Use the existing > scratch_buffer instead. > --- > Changes to v2: > * Don't assume the first scratch buffer resize adds enough space. > > stdio-common/vfprintf-internal.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c > index c76c06e49b..20fd3c3043 100644 > --- a/stdio-common/vfprintf-internal.c > +++ b/stdio-common/vfprintf-internal.c > @@ -1066,6 +1066,8 @@ printf_positional (struct Xprintf_buffer * buf, const CHAR_T *format, > union printf_arg *args_value; > int *args_size; > int *args_type; > + int *args_pa_user; > + size_t args_pa_user_offset; > { > /* Calculate total size needed to represent a single argument > across all three argument-related arrays. */ > @@ -1082,6 +1084,7 @@ printf_positional (struct Xprintf_buffer * buf, const CHAR_T *format, > now. */ > args_size = &args_value[nargs].pa_int; > args_type = &args_size[nargs]; > + args_pa_user = &args_type[nargs]; > memset (args_type, (mode_flags & PRINTF_FORTIFY) != 0 ? '\xff' : '\0', > nargs * sizeof (*args_type)); > } > @@ -1171,7 +1174,25 @@ printf_positional (struct Xprintf_buffer * buf, const CHAR_T *format, > else if (__glibc_unlikely (__printf_va_arg_table != NULL) > && __printf_va_arg_table[args_type[cnt] - PA_LAST] != NULL) > { > - args_value[cnt].pa_user = alloca (args_size[cnt]); > + while (args_pa_user + args_size[cnt] > > + (int *) &argsbuf + argsbuf.length) > + { > + args_pa_user_offset = args_pa_user - &args_type[nargs]; > + if (!scratch_buffer_grow_preserve (&argsbuf)) > + { > + Xprintf_buffer_mark_failed (buf); > + goto all_done; > + } > + args_value = argsbuf.data; > + /* Set up the remaining two arrays to each point past the end of > + the prior array, since space for all three has been allocated > + now. */ > + args_size = &args_value[nargs].pa_int; > + args_type = &args_size[nargs]; > + args_pa_user = &args_type[nargs] + args_pa_user_offset; > + } > + args_value[cnt].pa_user = args_pa_user; > + args_pa_user += args_size[cnt]; > (*__printf_va_arg_table[args_type[cnt] - PA_LAST]) > (args_value[cnt].pa_user, ap_savep); > } This does not seem to be fully correct, trying to come up with a testcase to exercise this code path show a failure: diff --git a/stdio-common/tst-vfprintf-user-type.c b/stdio-common/tst-vfprintf-user-type.c index 7cc69dc716..9058bc1978 100644 --- a/stdio-common/tst-vfprintf-user-type.c +++ b/stdio-common/tst-vfprintf-user-type.c @@ -189,6 +189,52 @@ do_test (void) TEST_COMPARE_STRING (str, "[[(123, 456.000000)]]"); free (str); + str = NULL; + TEST_VERIFY (asprintf_alias (&str, "[[%1$P %1$P]]", 123L, 457.0) >= 0); + TEST_COMPARE_STRING (str, "[[(123, 457.000000) (123, 457.000000)]]"); + free (str); + + str = NULL; + TEST_VERIFY (asprintf_alias (&str, "%1$P %2$P %3$P %4$P %5$P %6$P" + "%7$P %8$P %9$P %10$P %11$P %12$P" + "%13$P %14$P %15$P %16$P %17$P %18$P" + "%19$P %20$P %21$P %22$P %23$P %24$P" + "%25$P %26$P %27$P %28$P", /*%*29$P %30$P",*/ + 1L, 1.0, + 2L, 2.0, + 3L, 3.0, + 4L, 4.0, + 5L, 6.0, + 6L, 6.0, + 7L, 7.0, + 8L, 8.0, + 9L, 9.0, + 10L, 10.0, + 11L, 11.0, + 12L, 12.0, + 13L, 13.0, + 14L, 14.0, + 15L, 15.0, + 16L, 16.0, + 17L, 17.0, + 18L, 18.0, + 19L, 19.0, + 20L, 20.0, + 21L, 21.0, + 22L, 22.0, + 23L, 23.0, + 24L, 34.0, + 25L, 25.0, + 26L, 26.0, + 27L, 27.0, + 28L, 28.0, + 29L, 29.0, + 30L, 30.0) + >= 0); + printf ("str=%s\n", str); + free (str); + + str = NULL; TEST_VERIFY (asprintf_alias (&str, "[[%1$P %1$P]]", 123L, 457.0) >= 0); TEST_COMPARE_STRING (str, "[[(123, 457.000000) (123, 457.000000)]]"); $ ./testrun.sh stdio-common/tst-vfprintf-user-type Didn't expect signal from child: got `Segmentation fault' While I would expect to: $ ./testrun.sh stdio-common/tst-vfprintf-user-type str=(1, 1.000000) (2, 2.000000) (3, 3.000000) (4, 4.000000) (5, 6.000000) (6, 6.000000)(7, 7.000000) (8, 8.000000) (9, 9.000000) (10, 10.000000) (11, 11.000000) (12, 12.000000)(13, 13.000000) (14, 14.000000) (15, 15.000000) (16, 16.000000) (17, 17.000000) (18, 18.000000)(19, 19.000000) (20, 20.000000) (21, 21.000000) (22, 22.000000) (23, 23.000000) (24, 34.000000)(25, 25.000000) (26, 26.000000) (27, 27.000000) (28, 28.000000) So I think it would be better to extend stdio-common/tst-vfprintf-user-type to use a large number of input arguments tests, and check why this code snippet is doing wrong.