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 [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id B792F38515D4 for ; Tue, 22 Jun 2021 03:17:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B792F38515D4 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-307-IJKeEUOVMxW_Al7aZwu9AQ-1; Mon, 21 Jun 2021 23:17:47 -0400 X-MC-Unique: IJKeEUOVMxW_Al7aZwu9AQ-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 769C4802E62; Tue, 22 Jun 2021 03:17:46 +0000 (UTC) Received: from greed.delorie.com (ovpn-112-111.rdu2.redhat.com [10.10.112.111]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 294B610016FE; Tue, 22 Jun 2021 03:17:46 +0000 (UTC) Received: from greed.delorie.com.redhat.com (localhost [127.0.0.1]) by greed.delorie.com (8.15.2/8.15.2) with ESMTP id 15M3HiDL408296; Mon, 21 Jun 2021 23:17:44 -0400 From: DJ Delorie To: Siddhesh Poyarekar Cc: libc-alpha@sourceware.org Subject: Re: [PATCH 1/6] iconv: Remove alloca use in gconv-modules configuration parsing In-Reply-To: <20210610111853.2286873-2-siddhesh@sourceware.org> Date: Mon, 21 Jun 2021 23:17:44 -0400 Message-ID: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-12.5 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_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Tue, 22 Jun 2021 03:17:51 -0000 Siddhesh Poyarekar writes: > The alloca sizes ought to be constrained to PATH_MAX, but replace them > with dynamic allocation to be safe. A static PATH_MAX array would > have worked too but Hurd does not have PATH_MAX and the code path is > not hot enough to micro-optimise this allocation. Revisit if any of > those realities change. > --- > iconv/gconv_conf.c | 17 +++++++++-------- > iconv/iconvconfig.c | 17 +++++++++++------ > 2 files changed, 20 insertions(+), 14 deletions(-) > > diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c > index c8ad8099a4..3f2cef255b 100644 > --- a/iconv/gconv_conf.c > +++ b/iconv/gconv_conf.c > @@ -559,15 +559,15 @@ __gconv_read_conf (void) > > for (cnt = 0; __gconv_path_elem[cnt].name != NULL; ++cnt) > { > -#define BUF_LEN elem_len + sizeof (gconv_conf_dirname) > - > const char *elem = __gconv_path_elem[cnt].name; > size_t elem_len = __gconv_path_elem[cnt].len; > - char *buf; > > /* No slash needs to be inserted between elem and gconv_conf_filename; > elem already ends in a slash. */ > - buf = alloca (BUF_LEN); > + char *buf = malloc (elem_len + sizeof (gconv_conf_dirname)); > + if (buf == NULL) > + continue; > + > char *cp = __mempcpy (__mempcpy (buf, elem, elem_len), > gconv_conf_filename, sizeof (gconv_conf_filename)); > replaces alloca with malloc; sizes are the same. Ok. > @@ -596,15 +596,16 @@ __gconv_read_conf (void) > if (len > strlen (suffix) > && strcmp (ent->d_name + len - strlen (suffix), suffix) == 0) > { > - /* LEN <= PATH_MAX so this alloca is not unbounded. */ > - char *conf = alloca (BUF_LEN + len + 1); > - cp = stpcpy (conf, buf); > - sprintf (cp, "/%s", ent->d_name); > + char *conf; > + if (__asprintf (&conf, "%s/%s", buf, ent->d_name) < 0) > + continue; > read_conf_file (conf, elem, elem_len, &modules, &nmodules); > + free (conf); allocated by asprintf, free'd here. Ok. > } > } > __closedir (confdir); > } > + free (buf); Matched free, OK. > } > #endif > > diff --git a/iconv/iconvconfig.c b/iconv/iconvconfig.c > index b2a868919c..c9607fb645 100644 > --- a/iconv/iconvconfig.c > +++ b/iconv/iconvconfig.c > @@ -712,7 +712,6 @@ handle_file (const char *dir, const char *infile) > static int > handle_dir (const char *dir) > { > -#define BUF_LEN prefix_len + dirlen + sizeof "gconv-modules.d" > char *cp; > size_t dirlen = strlen (dir); > bool found = false; > @@ -726,7 +725,10 @@ handle_dir (const char *dir) > } > > /* First, look for a gconv-modules file. */ > - char buf[BUF_LEN]; > + char *buf = malloc (prefix_len + dirlen + sizeof "gconv-modules.d"); > + if (buf == NULL) > + goto out; > + Likewise, ok. > cp = buf; > if (dir[0] == '/') > cp = mempcpy (cp, prefix, prefix_len); > @@ -756,16 +758,19 @@ handle_dir (const char *dir) > if (len > strlen (suffix) > && strcmp (ent->d_name + len - strlen (suffix), suffix) == 0) > { > - /* LEN <= PATH_MAX so this alloca is not unbounded. */ > - char *conf = alloca (BUF_LEN + len + 1); > - cp = stpcpy (conf, buf); > - sprintf (cp, "/%s", ent->d_name); > + char *conf; > + if (asprintf (&conf, "%s/%s", buf, ent->d_name) < 0) > + continue; > found |= handle_file (dir, conf); > + free (conf); Likewise, ok. > } > } > closedir (confdir); > } > > + free (buf); > + > +out: Freed, ok. > if (!found) > { > error (0, errno, "failed to open gconv configuration files in `%s'", LGTM. Reviewed-by: DJ Delorie