From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 575B53858D39 for ; Thu, 21 Oct 2021 09:54:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 575B53858D39 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-94-C80paU9OP_iV80m3SUvtKA-1; Thu, 21 Oct 2021 05:54:52 -0400 X-MC-Unique: C80paU9OP_iV80m3SUvtKA-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 607829126B; Thu, 21 Oct 2021 09:54:51 +0000 (UTC) Received: from oldenburg.str.redhat.com (unknown [10.39.194.104]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E7FBDADD8; Thu, 21 Oct 2021 09:54:49 +0000 (UTC) From: Florian Weimer To: Adhemerval Zanella Cc: Adhemerval Zanella via Libc-alpha , Paul Eggert , bug-gnulib@gnu.org Subject: Re: [PATCH 2/2] posix: Remove alloca usage for internal fnmatch implementation References: <20210104202528.1228255-1-adhemerval.zanella@linaro.org> <20210104202528.1228255-2-adhemerval.zanella@linaro.org> <87h7ller7l.fsf@oldenburg.str.redhat.com> Date: Thu, 21 Oct 2021 11:54:47 +0200 In-Reply-To: (Adhemerval Zanella's message of "Wed, 20 Oct 2021 12:12:24 -0300") Message-ID: <87sfwusnx4.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-6.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Oct 2021 09:54:57 -0000 * Adhemerval Zanella: > On 08/03/2021 09:59, Florian Weimer wrote: >> * Adhemerval Zanella via Libc-alpha: >> >>> - else if (*p == L_('|')) >>> + else if (*p == L_(')') || *p == L_('|')) >>> { >>> if (level == 0) >>> { >>> - NEW_PATTERN; >>> - startp = p + 1; >>> + size_t slen = opt == L_('?') || opt == L_('@') >>> + ? pattern_len : p - startp + 1; >>> + CHAR *newp = malloc (slen * sizeof (CHAR)); >>> + if (newp != NULL) >>> + { >>> + *((CHAR *) MEMPCPY (newp, startp, p - startp)) = L_('\0'); >>> + PASTE (PATTERN_PREFIX,_add) (&list, newp); >>> + } >>> + if (newp == NULL || PASTE (PATTERN_PREFIX, _has_failed) (&list)) >>> + { >>> + retval = -2; >>> + goto out; >>> + } >>> + >>> + if (*p == L_('|')) >>> + startp = p + 1; >>> } >> >> slen seems to be the wrong variable name. But I don't know wh the >> original code computes plen conditionally and then uses p - startp >> unconditionally. That seems wrong. The discrepancy goes back to >> 821a6bb4360. Do you see a case where the difference matters? >> >> The == 0 checks for the recursive FCT calls are wrong because they treat >> match failure the same as OOM and other errors (the -2 return value), >> but that also is a pre-existing issue. >> >> The conversation itself appears to be faithful. > > Hi Florian, > > I noted this patch [1] is marked accepted, was you the one that > accepted it? In any case, are you still ok with the change? > > > [1] https://patchwork.sourceware.org/project/glibc/patch/20210202130804.1920933-2-adhemerval.zanella@linaro.org/ I think all the issues I identified are pre-existing, and as I said, the conversion to remove alloca appears to be correct. Thanks, Florian