From patchwork Wed Jan 22 12:48:56 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 105229 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 571F23857810 for ; Wed, 22 Jan 2025 12:49:55 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 571F23857810 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Mw3jMurF X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTP id 1FD3A3858C5F for ; Wed, 22 Jan 2025 12:49:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1FD3A3858C5F Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 1FD3A3858C5F Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1737550146; cv=none; b=kjRwYJR80cnL6Oo7ZqwlSb7wdZHPC+HBiIkJdb83J4xkDYiELYaGmm/NF5jQzIvraqIaqBNEaypD1qbE39Y+6mwAuKTpHu1TAd16aTNS/R64+aW1MTdzyEvWSk8zmyd1AIAv7iKQpPr69clvEFbC3Gm39lFmaEQ/ZF8UsW3Y5kI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1737550146; c=relaxed/simple; bh=iMAWYJe3bQx6gHTsT7Q9ZPC9oH0I/OwMNHHVMbtHDWQ=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=al8f4FQR+CdFO+SbV/vwKkIp7LapB9hJnjfmTctM8ZHLSlLKRlmywLVccnIORUuKos5opr4ULoi/wM4ThgA7XZFOIJZVTCl62k6BwOtxDZtNl/8JXnOt376g115BdR4/N7aXm1AaackqstxzZMzDmMV08vDvk0n0SI9HIyvyrqY= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 1FD3A3858C5F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1737550145; 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; bh=jKZm3GvrzXApIBL7i1ZZAPVA3NZoH+BB0kg4wtvfjPk=; b=Mw3jMurFc30oCnbPzEK9wiamVHN2rsYZ65i2QsioXysfJUrpNPIbs45atJFZvPgnAhhHXL bRM6B05Okz/n9ogjUqVQV+4mgATBDIteiDShO7548XTBUUj5YJmHDhOGwNaL9ym8nSOhUJ 0uReMqvT4/BD4ZfXH5VfXiwWnImmZ68= Received: from mx-prod-mc-03.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-403-0M74HS2cNkKzc398BJvc3A-1; Wed, 22 Jan 2025 07:49:02 -0500 X-MC-Unique: 0M74HS2cNkKzc398BJvc3A-1 X-Mimecast-MFC-AGG-ID: 0M74HS2cNkKzc398BJvc3A Received: from mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.40]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-03.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id BCEAA1955D61; Wed, 22 Jan 2025 12:49:00 +0000 (UTC) Received: from oldenburg.str.redhat.com (unknown [10.2.16.57]) by mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id A582319560AA; Wed, 22 Jan 2025 12:48:58 +0000 (UTC) From: Florian Weimer To: libc-alpha@sourceware.org Cc: =?utf-8?q?Andreas_K=2E_H=C3=BCttel?= Subject: [PATCH] stdlib: Support malloc-managed environ arrays for compatibility Date: Wed, 22 Jan 2025 13:48:56 +0100 Message-ID: <87jzan8153.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.0 on 10.30.177.40 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 0QTl1n8g_rk0r95tQu8u0BlNoOP58FstrnqwyhEaAO8_1737550142 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5, 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 X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libc-alpha-bounces~patchwork=sourceware.org@sourceware.org Some allocations set environ to a heap-allocated pointer, call setenv (expecting it to call realloc), free environ, and then restore the original environ pointer. This breaks after commit 7a61e7f557a97ab597d6fca5e2d1f13f65685c61 ("stdlib: Make getenv thread-safe in more cases") because after the setenv call, the environ pointer does not point to the start of a heap allocation. Instead, setenv creates a separate allocation and changes environ to point into that. This means that the free call in the application results in heap corruption. The interim approach was more compatible with other libcs because it does not assume that the incoming environ pointer is allocated as if by malloc (if it was written by the application). However, it seems to be more important to stay compatible with previous glibc version: assume the incoming pointer is heap allocated, and preserve this property after setenv calls. Tested on x86_64-linux-gnu. I'd prefer to get this into the upcoming release if possible, assuming that I can get a review in a timely fashion. Thanks, Florian Reviewed-by: Carlos O'Donell --- csu/init-first.c | 1 + csu/libc-start.c | 1 + include/unistd.h | 3 +++ posix/environ.c | 2 ++ stdlib/Makefile | 1 + stdlib/setenv.c | 64 +++++++++++++++++++++++----------------------- stdlib/tst-setenv-malloc.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 104 insertions(+), 32 deletions(-) base-commit: d4626340b997d662ddca4a48f8a3bf03ccb7a603 diff --git a/csu/init-first.c b/csu/init-first.c index e35e4ce84f..0ad6f75dcd 100644 --- a/csu/init-first.c +++ b/csu/init-first.c @@ -61,6 +61,7 @@ _init_first (int argc, char **argv, char **envp) __libc_argc = argc; __libc_argv = argv; __environ = envp; + __environ_startup = envp; #ifndef SHARED /* First the initialization which normally would be done by the diff --git a/csu/libc-start.c b/csu/libc-start.c index 6f3d52e223..4e15b6191d 100644 --- a/csu/libc-start.c +++ b/csu/libc-start.c @@ -244,6 +244,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL), char **ev = &argv[argc + 1]; __environ = ev; + __environ_startup = ev; /* Store the lowest stack address. This is done in ld.so if this is the code for the DSO. */ diff --git a/include/unistd.h b/include/unistd.h index e241603b81..ada957f9d0 100644 --- a/include/unistd.h +++ b/include/unistd.h @@ -203,6 +203,9 @@ libc_hidden_proto (__tcsetpgrp) extern int __libc_enable_secure attribute_relro; rtld_hidden_proto (__libc_enable_secure) +/* Original value of __environ. Initialized by _init_first (dynamic) + or __libc_start_main (static). */ +extern char **__environ_startup attribute_hidden; /* Various internal function. */ extern void __libc_check_standard_fds (void) attribute_hidden; diff --git a/posix/environ.c b/posix/environ.c index a0ed0d80ea..2430b47d8e 100644 --- a/posix/environ.c +++ b/posix/environ.c @@ -10,3 +10,5 @@ weak_alias (__environ, environ) /* The SVR4 ABI says `_environ' will be the name to use in case the user overrides the weak alias `environ'. */ weak_alias (__environ, _environ) + +char **__environ_startup; diff --git a/stdlib/Makefile b/stdlib/Makefile index a5fbc1a27e..ee95b2e79a 100644 --- a/stdlib/Makefile +++ b/stdlib/Makefile @@ -316,6 +316,7 @@ tests := \ tst-setcontext9 \ tst-setcontext10 \ tst-setcontext11 \ + tst-setenv-malloc \ tst-stdbit-Wconversion \ tst-stdbit-builtins \ tst-stdc_bit_ceil \ diff --git a/stdlib/setenv.c b/stdlib/setenv.c index 2a2eec9c98..c6dc9f7945 100644 --- a/stdlib/setenv.c +++ b/stdlib/setenv.c @@ -191,52 +191,52 @@ __add_to_environ (const char *name, const char *value, const char *combined, ep[1] = NULL; else { - /* We cannot use __environ as is and need to copy over the - __environ contents into an array managed via - __environ_array_list. */ + /* We cannot use __environ as is and need a larger allocation. */ - struct environ_array *target_array; - if (__environ_array_list != NULL - && required_size <= __environ_array_list->allocated) - /* Existing array has enough room. Contents is copied below. */ - target_array = __environ_array_list; - else + if (start_environ == __environ_startup + || __environ_is_from_array_list (start_environ)) { - /* Allocate a new array. */ - target_array = __environ_new_array (required_size); + /* Allocate a new array, managed in the list. */ + struct environ_array *target_array + = __environ_new_array (required_size); if (target_array == NULL) { UNLOCK; return -1; } - } - - /* Copy over the __environ array contents. This forward - copy slides backwards part of the array if __environ - points into target_array->array. This happens if an - application makes an assignment like: - - environ = &environ[1]; + result_environ = &target_array->array[0]; - The forward copy avoids clobbering values that still - needing copying. This code handles the case - start_environ == ep == NULL, too. */ - size_t i; - for (i = 0; start_environ + i < ep; ++i) - /* Regular store because unless there has been direct - manipulation of the environment, target_array is still - a private copy. */ - target_array->array[i] = atomic_load_relaxed (start_environ + i); + /* Copy over the __environ array contents. This code + handles the case start_environ == ep == NULL, too. */ + size_t i; + for (i = 0; start_environ + i < ep; ++i) + /* Regular store because unless there has been direct + manipulation of the environment, target_array is still + a private copy. */ + result_environ[i] = atomic_load_relaxed (start_environ + i); + } + else + { + /* Otherwise the application installed its own pointer. + Historically, this pointer was managed using realloc. + Continue doing so. This disables multi-threading + support. */ + result_environ = __libc_reallocarray (start_environ, + required_size, + sizeof (*result_environ)); + if (result_environ == NULL) + { + UNLOCK; + return -1; + } + } /* This is the new place where we should add the element. */ - ep = target_array->array + i; + ep = result_environ + (required_size - 2); /* Add the null terminator in case there was a pointer there previously. */ ep[1] = NULL; - - /* And __environ should be repointed to our array. */ - result_environ = &target_array->array[0]; } } diff --git a/stdlib/tst-setenv-malloc.c b/stdlib/tst-setenv-malloc.c new file mode 100644 index 0000000000..18a9d36842 --- /dev/null +++ b/stdlib/tst-setenv-malloc.c @@ -0,0 +1,64 @@ +/* Test using setenv with a malloc-allocated environ variable. + Copyright (C) 2025 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +/* This test is not in the scope for POSIX or any other standard, but + some applications assume that environ is a heap-allocated pointer + after a call to setenv on an empty environment. */ + +#include +#include +#include +#include + +static const char *original_path; +static char **save_environ; + +static void +rewrite_environ (void) +{ + save_environ = environ; + environ = xmalloc (sizeof (*environ)); + *environ = NULL; + TEST_COMPARE (setenv ("A", "1", 1), 0); + TEST_COMPARE (setenv ("B", "2", 1), 0); + TEST_VERIFY (environ != save_environ); + TEST_COMPARE_STRING (environ[0], "A=1"); + TEST_COMPARE_STRING (environ[1], "B=2"); + TEST_COMPARE_STRING (environ[2], NULL); + TEST_COMPARE_STRING (getenv ("PATH"), NULL); + free (environ); + environ = save_environ; + TEST_COMPARE_STRING (getenv ("PATH"), original_path); +} + +static int +do_test (void) +{ + original_path = getenv ("PATH"); + rewrite_environ (); + + /* Test again after reallocated the environment due to an initial + setenv call. */ + TEST_COMPARE (setenv ("TST_SETENV_MALLOC", "1", 1), 0); + TEST_VERIFY (environ != save_environ); + rewrite_environ (); + + return 0; +} + +#include