From patchwork Wed Sep 4 22:18:50 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joseph Myers X-Patchwork-Id: 97111 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 C74CB3850212 for ; Wed, 4 Sep 2024 22:20:03 +0000 (GMT) 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 0F0E33858424 for ; Wed, 4 Sep 2024 22:19:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0F0E33858424 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 0F0E33858424 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=1725488379; cv=none; b=B+yZ/kcrJQIR3tg/F2AdPfVUR4Gvinr9ZqIBrtt2Yefb/OKlhDrtdiXf6Qd49AHPr2tQAAygHI2lf+br6Fm1cSMpAlmNz//q5S2UnDvTfQ65vhya2GpSumP+RTsyD3aKrjS88tQu6YsusT+aggOFtHIPRIRelIIB47JOirwrMPE= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1725488379; c=relaxed/simple; bh=9TI4Fpi82CekUeJWkb2nFFbow6DwayZbOYeUZyks9OM=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=sPJfAx/Z0QjPy4EPMKTaT3VAFAXGZQnrGWQnOdqHbz9FLhA5YA2iyA5gigmMs3pE++F7m+jgucmALFboeN8/Tl/pqGKVaQF6Bh1kaORUSK1oZep2XXsWc8gFwgxFE0XSoxUYgOhGUSpLCU9XX+Ia4s27fXGcJWtQiiaD6Bi9axo= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1725488376; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type; bh=jBeWoKu6FmGMgKa5wz2jwe5UIkse1BMe5ySqMs5ylzo=; b=VRq65241XyQOf7j2URR4fDQnJHtiqsK0bvLly6CdSGuILSlSOx2jC85c+sO2L2i/VB1pkH CR5UIGtyDK5QDeX9s5gm7xd3livgERVzRP580reTu84iBpvSezfApp5PLCa4DDvpztZVG7 flzSx2uFoiVInjIHC+yHSKc0t7c9BlY= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-147-lcDTqg8_Pcmhy74A7qjRww-1; Wed, 04 Sep 2024 18:19:35 -0400 X-MC-Unique: lcDTqg8_Pcmhy74A7qjRww-1 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-42bb9fa67c5so150775e9.1 for ; Wed, 04 Sep 2024 15:19:35 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725488374; x=1726093174; h=mime-version:message-id:subject:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=jBeWoKu6FmGMgKa5wz2jwe5UIkse1BMe5ySqMs5ylzo=; b=nwSyGcwOddLGTPjHvdDymDQE1loRRFXRvfgdf7oU0/0wCVB6OGBoaKQag9Tp1IhOkm I8OCs35a8IptRTdJAnVdH7MoT2BoYVGkXj6cb1OIu4HzUpVa5lDAqFAFGxXGjGOck5I0 qYrIDQak1mlnUyvzKD757W4P/ko/YPGt839eZJgkKaLRBKydOlbedQjfWUFhJf3mAocY tYf17H1aCZKhxY+XJE5VQu32q8DtgprgkvijXA6jbQMYzcmusvF5xFEEoY1113wx3sHU fjS8A/YzBdnA4JanSHFJWT9E/hF+hlx11z7Kgr3Eb4KnaGkMzJUhN7xExB6hzwX8imKa /EQw== X-Gm-Message-State: AOJu0YyuQ+NLTbw08ViXVpR7ZmiMJVpfXhK+aVYFdwyY5nOxFyYwBSKb 98yR/mKsx88cLasmfy61I5YeAIZI7d9a5CLfMUoWN5VbnaUBpd93Dz5mfmjlRDtUUxcIEa9/tyj XrJtxjADgc/qTOmyhCoYFytA42MTgoYt7wa36qA7Wxh19RvEGG+PxvLqQZSGqxaXWNlyIAY41rr 2U0zKC3x2kpNK0pJVsaDpl0UeBj15rIHaTMB6WbrFJ2Q== X-Received: by 2002:a05:600c:4f42:b0:426:5269:9827 with SMTP id 5b1f17b1804b1-42bb01363ecmr158298305e9.0.1725488374053; Wed, 04 Sep 2024 15:19:34 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEsIkNiu113DP2GUSU+Z8zgYJSofOIegoV5VmbaXUeexxB+GMZPGKUQn/A4yWc2IKuM6DYGug== X-Received: by 2002:a05:600c:4f42:b0:426:5269:9827 with SMTP id 5b1f17b1804b1-42bb01363ecmr158298165e9.0.1725488373051; Wed, 04 Sep 2024 15:19:33 -0700 (PDT) Received: from digraph.polyomino.org.uk (digraph.polyomino.org.uk. [2001:8b0:bf73:93f7::51bb:e332]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-42bb6e355dfsm215202455e9.46.2024.09.04.15.19.32 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 04 Sep 2024 15:19:32 -0700 (PDT) Received: from jsm28 (helo=localhost) by digraph.polyomino.org.uk with local-esmtp (Exim 4.95) (envelope-from ) id 1slyKw-006zoR-BY for libc-alpha@sourceware.org; Wed, 04 Sep 2024 22:18:50 +0000 Date: Wed, 4 Sep 2024 22:18:50 +0000 (UTC) From: Joseph Myers To: libc-alpha@sourceware.org Subject: Fix memory leak on freopen error return (bug 32140) Message-ID: MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-9.1 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_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, 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 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 As reported in bug 32140, freopen leaks the FILE object when it returns NULL: there is no valid use of the FILE * pointer (including passing to freopen again or to fclose) after such an error return, so the underlying object should be freed. Add code to free it. Note 1: while I think it's clear from the relevant standards that the object should be freed and the FILE * can't be used after the call in this case (the stream is closed, which ends the lifetime of the FILE), it's entirely possible that some existing code does in fact try to use the existing FILE * in some way and could be broken by this change. (Though the most common case for freopen may be stdin / stdout / stderr, which _IO_deallocate_file explicitly checks for and does not deallocate.) Note 2: the deallocation is only done in the _IO_IS_FILEBUF case. Other kinds of streams bypass all the freopen logic handling closing the file, meaning a call to _IO_deallocate_file would neither be safe (the FILE might still be linked into the list of all open FILEs) nor sufficient (other internal memory allocations associated with the file would not have been freed). I think the validity of freopen for any other kind of stream will need clarifying with the Austin Group, but if it is valid in any such case (where "valid" means "not undefined behavior so required to close the stream" rather than "required to successfully associate the stream with the new file in cases where fopen would work"), more significant changes would be needed to ensure the stream gets fully closed. Tested for x86_64. Reviewed-by: Florian Weimer diff --git a/libio/freopen.c b/libio/freopen.c index c7e36db775..301c64ddc3 100644 --- a/libio/freopen.c +++ b/libio/freopen.c @@ -108,5 +108,7 @@ freopen (const char *filename, const char *mode, FILE *fp) end: _IO_release_lock (fp); + if (result == NULL && (fp->_flags & _IO_IS_FILEBUF) != 0) + _IO_deallocate_file (fp); return result; } diff --git a/libio/freopen64.c b/libio/freopen64.c index 9a6d5ed801..1700778622 100644 --- a/libio/freopen64.c +++ b/libio/freopen64.c @@ -91,5 +91,7 @@ freopen64 (const char *filename, const char *mode, FILE *fp) end: _IO_release_lock (fp); + if (result == NULL && (fp->_flags & _IO_IS_FILEBUF) != 0) + _IO_deallocate_file (fp); return result; } diff --git a/stdio-common/Makefile b/stdio-common/Makefile index 03d597f8e6..d99e0cbfeb 100644 --- a/stdio-common/Makefile +++ b/stdio-common/Makefile @@ -321,7 +321,9 @@ ifeq (yes,$(build-shared)) ifneq ($(PERL),no) tests-special += \ $(objpfx)tst-freopen2-mem.out \ + $(objpfx)tst-freopen3-mem.out \ $(objpfx)tst-freopen64-2-mem.out \ + $(objpfx)tst-freopen64-3-mem.out \ $(objpfx)tst-getline-enomem-mem.out \ $(objpfx)tst-getline-mem.out \ $(objpfx)tst-printf-bz18872-mem.out \ @@ -335,8 +337,12 @@ tests-special += \ generated += \ tst-freopen2-mem.out \ tst-freopen2.mtrace \ + tst-freopen3-mem.out \ + tst-freopen3.mtrace \ tst-freopen64-2-mem.out \ tst-freopen64-2.mtrace \ + tst-freopen64-3-mem.out \ + tst-freopen64-3.mtrace \ tst-getline-enomem-mem.out \ tst-getline-enomem.mtrace \ tst-getline-mem.out \ @@ -462,6 +468,12 @@ tst-freopen2-ENV = \ tst-freopen64-2-ENV = \ MALLOC_TRACE=$(objpfx)tst-freopen64-2.mtrace \ LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so +tst-freopen3-ENV = \ + MALLOC_TRACE=$(objpfx)tst-freopen3.mtrace \ + LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so +tst-freopen64-3-ENV = \ + MALLOC_TRACE=$(objpfx)tst-freopen64-3.mtrace \ + LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so $(objpfx)tst-unbputc.out: tst-unbputc.sh $(objpfx)tst-unbputc $(SHELL) $< $(common-objpfx) '$(test-program-prefix)'; \ diff --git a/stdio-common/tst-freopen3-main.c b/stdio-common/tst-freopen3-main.c index 5107e1f98e..990a6e5921 100644 --- a/stdio-common/tst-freopen3-main.c +++ b/stdio-common/tst-freopen3-main.c @@ -18,6 +18,7 @@ #include #include +#include #include #include @@ -48,6 +49,7 @@ int do_test (void) { + mtrace (); struct support_descriptors *fds; char *temp_dir = support_create_temp_directory ("tst-freopen3"); char *file1 = xasprintf ("%s/file1", temp_dir);