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.129.124]) by sourceware.org (Postfix) with ESMTPS id 290C43858D33 for ; Mon, 28 Aug 2023 14:58:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 290C43858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1693234710; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=nP4hI0gOU3Yf4M0SmlygMCkXdm5ge4o3lRIiMiHS0M4=; b=Rni9lLY462IM42irluVmJudUZqrbSeir6nONdIRSA/B01V0HZ/nGXK2ZnJ81QR18OM5tz/ PiDRlEPLVBdHwS5oZdp9VfdWxEelJgkYLCvM+TUkJZt47ns/hfyXZTYGxJXSjlEtQUwncV ruMYA3F4QkbDD//gTae4VMYVrl9OjBg= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-499-ZTCq8F_pM9a2gwbTSuURHw-1; Mon, 28 Aug 2023 10:58:27 -0400 X-MC-Unique: ZTCq8F_pM9a2gwbTSuURHw-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 48492185A791; Mon, 28 Aug 2023 14:58:27 +0000 (UTC) Received: from oak (unknown [10.22.33.147]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 2B71240C2063; Mon, 28 Aug 2023 14:58:27 +0000 (UTC) Date: Mon, 28 Aug 2023 10:58:25 -0400 From: Joe Simmons-Talbott To: Adhemerval Zanella Netto Cc: libc-alpha@sourceware.org Subject: Re: [PATCH] resolv/res_query: Add note indicating that alloca usage is safe. Message-ID: <20230828145825.GX3849957@oak> References: <20230705181341.1470594-1-josimmon@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP 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 Mon, Aug 28, 2023 at 10:50:21AM -0300, Adhemerval Zanella Netto wrote: > > > On 05/07/23 15:13, Joe Simmons-Talbott via Libc-alpha wrote: > > The buffer size is small (< 1024) and fixed sized so alloca is safe > > here. > > --- > > resolv/res_query.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/resolv/res_query.c b/resolv/res_query.c > > index 049de91b95..0e0e7be624 100644 > > --- a/resolv/res_query.c > > +++ b/resolv/res_query.c > > @@ -117,6 +117,7 @@ __res_context_query (struct resolv_context *ctx, const char *name, > > int n, use_malloc = 0; > > > > size_t bufsize = (type == T_QUERY_A_AND_AAAA ? 2 : 1) * QUERYSIZE; > > + /* alloca is safe here since bufsize < 1024 and fixed sized. */ > > u_char *buf = alloca (bufsize); > > The bufsize on current Linux build is: > > size_t bufsize = (type == 439963904 ? 2 : 1) * (12 + 4 + 255 + 1); > > So with upper bound as 544 (2 * (12 + 4 + 255 + 1)). However, it might > increase to 2 * PACKETSIZE later with malloc. This is exactly the scenarion > scratch_buffer was created, so maybe we should use it. Below a complete > untested patch: Thanks for the patch. I've tested it with 'make check' on x86_64-linux-gnu. I'm not sure how to handle this and don't feel comfortable submitting it on your behalf or as my own work but am happy to give a Reviewed-by once there's a commit message. Thanks, Joe > > diff --git a/resolv/res_query.c b/resolv/res_query.c > index 049de91b95..b234db83c1 100644 > --- a/resolv/res_query.c > +++ b/resolv/res_query.c > @@ -80,6 +80,7 @@ > #include > #include > #include > +#include > > #if PACKETSZ > 65536 > #define MAXPACKET PACKETSZ > @@ -114,11 +115,14 @@ __res_context_query (struct resolv_context *ctx, const char *name, > struct __res_state *statp = ctx->resp; > UHEADER *hp = (UHEADER *) answer; > UHEADER *hp2; > - int n, use_malloc = 0; > - > - size_t bufsize = (type == T_QUERY_A_AND_AAAA ? 2 : 1) * QUERYSIZE; > - u_char *buf = alloca (bufsize); > - u_char *query1 = buf; > + int n; > + > + /* It requires 2 times QUERYSIZE for type == T_QUERY_A_AND_AAAA. */ > + struct scratch_buffer buf; > + scratch_buffer_init (&buf); > + _Static_assert (2 * QUERYSIZE <= sizeof (buf.__space.__c), > + "scratch_buffer too small"); > + u_char *query1 = buf.data; > int nquery1 = -1; > u_char *query2 = NULL; > int nquery2 = 0; > @@ -129,14 +133,14 @@ __res_context_query (struct resolv_context *ctx, const char *name, > if (type == T_QUERY_A_AND_AAAA) > { > n = __res_context_mkquery (ctx, QUERY, name, class, T_A, NULL, > - query1, bufsize); > + query1, buf.length); > if (n > 0) > { > if ((statp->options & (RES_USE_EDNS0|RES_USE_DNSSEC)) != 0) > { > /* Use RESOLV_EDNS_BUFFER_SIZE because the receive > buffer can be reallocated. */ > - n = __res_nopt (ctx, n, query1, bufsize, > + n = __res_nopt (ctx, n, query1, buf.length, > RESOLV_EDNS_BUFFER_SIZE); > if (n < 0) > goto unspec_nomem; > @@ -146,20 +150,20 @@ __res_context_query (struct resolv_context *ctx, const char *name, > /* Align the buffer. */ > int npad = ((nquery1 + __alignof__ (HEADER) - 1) > & ~(__alignof__ (HEADER) - 1)) - nquery1; > - if (n > bufsize - npad) > + if (n > buf.length - npad) > { > n = -1; > goto unspec_nomem; > } > int nused = n + npad; > - query2 = buf + nused; > + query2 = buf.data + nused; > n = __res_context_mkquery (ctx, QUERY, name, class, T_AAAA, > - NULL, query2, bufsize - nused); > + NULL, query2, buf.length - nused); > if (n > 0 > && (statp->options & (RES_USE_EDNS0|RES_USE_DNSSEC)) != 0) > /* Use RESOLV_EDNS_BUFFER_SIZE because the receive > buffer can be reallocated. */ > - n = __res_nopt (ctx, n, query2, bufsize, > + n = __res_nopt (ctx, n, query2, buf.length, > RESOLV_EDNS_BUFFER_SIZE); > nquery2 = n; > } > @@ -169,7 +173,7 @@ __res_context_query (struct resolv_context *ctx, const char *name, > else > { > n = __res_context_mkquery (ctx, QUERY, name, class, type, NULL, > - query1, bufsize); > + query1, buf.length); > > if (n > 0 > && (statp->options & (RES_USE_EDNS0|RES_USE_DNSSEC)) != 0) > @@ -181,27 +185,25 @@ __res_context_query (struct resolv_context *ctx, const char *name, > advertise = anslen; > else > advertise = RESOLV_EDNS_BUFFER_SIZE; > - n = __res_nopt (ctx, n, query1, bufsize, advertise); > + n = __res_nopt (ctx, n, query1, buf.length, advertise); > } > > nquery1 = n; > } > > - if (__glibc_unlikely (n <= 0) && !use_malloc) { > + if (__glibc_unlikely (n <= 0)) { > /* Retry just in case res_nmkquery failed because of too > short buffer. Shouldn't happen. */ > - bufsize = (type == T_QUERY_A_AND_AAAA ? 2 : 1) * MAXPACKET; > - buf = malloc (bufsize); > - if (buf != NULL) { > - query1 = buf; > - use_malloc = 1; > + if (scratch_buffer_set_array_size (&buf, > + T_QUERY_A_AND_AAAA ? 2 : 1, > + MAXPACKET)) { > + query1 = buf.data; > goto again; > } > } > if (__glibc_unlikely (n <= 0)) { > RES_SET_H_ERRNO(statp, NO_RECOVERY); > - if (use_malloc) > - free (buf); > + scratch_buffer_free (&buf); > return (n); > } > > @@ -224,8 +226,7 @@ __res_context_query (struct resolv_context *ctx, const char *name, > answerp2_malloced); > } > > - if (use_malloc) > - free (buf); > + scratch_buffer_free (&buf); > if (n < 0) { > RES_SET_H_ERRNO(statp, TRY_AGAIN); > return (n); >