From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io1-xd32.google.com (mail-io1-xd32.google.com [IPv6:2607:f8b0:4864:20::d32]) by sourceware.org (Postfix) with ESMTPS id 00FC53858C60 for ; Fri, 21 Jan 2022 23:14:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 00FC53858C60 Received: by mail-io1-xd32.google.com with SMTP id r204so8811330iod.10 for ; Fri, 21 Jan 2022 15:14:05 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:from :subject:to:cc:references:content-language:in-reply-to :content-transfer-encoding; bh=fp72gVkncGMo3EWR6jrCSs15krOZ6ixik47CUedxzXU=; b=mTBQNI4DJDUlCJbgnF1nNUllxEKBMqh7VGZFghTVqNFbMvy8DfX+7OBZZriJBDnwzo u8PpaT98vqykeQ9Tk/F4U7yASQnk0wGBScSIJXZCrz0JyFzWPhSvsJkyVlajpElSRlqS ZJSjyWBkvYrEqSr4KWGRhUy9L6sL6uu+sqbPNvnB69fffmdhbt/sJwjhtZRhbOexRpPg UMbIU0qdpW9ImfQHOcZCnPzbvTDPew1B6FRsRr43sUXt+fSUA1SVuEr/lMnnIx69cmq4 xB0VVAvenJSFgSg6WxLJDY6Sy7awTzWErfFFVLZ51xG3rCgJazUs1NOQeg4jvUsdDvqr KnDA== X-Gm-Message-State: AOAM531rYiyS88GcpNHE8uvgxrdMfusTiHCNsW1z0hyHh9QpFtcGQxND 9+CGFYYR1xEI9vrT0+M9XKc= X-Google-Smtp-Source: ABdhPJyVHTelBtG51Sa4WVPVhByFWMxEXgBPRk0o/sulZuuqCQAjfjnvtqtjCnQuRzBRvdTaFHFdAw== X-Received: by 2002:a02:878b:: with SMTP id t11mr2852695jai.315.1642806845325; Fri, 21 Jan 2022 15:14:05 -0800 (PST) Received: from [192.168.0.41] (97-118-100-142.hlrn.qwest.net. [97.118.100.142]) by smtp.gmail.com with ESMTPSA id s12sm3947107ilu.84.2022.01.21.15.14.04 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 21 Jan 2022 15:14:04 -0800 (PST) Message-ID: <6c44a5a2-ed1e-abfe-2ead-9ddb6a97b8fc@gmail.com> Date: Fri, 21 Jan 2022 16:14:04 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 From: Martin Sebor Subject: Re: [PATCH] avoid -Wuse-after-free [BZ #26779] To: Paul Eggert Cc: libc-alpha@sourceware.org References: Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, 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: Fri, 21 Jan 2022 23:14:10 -0000 On 1/15/22 19:25, Paul Eggert wrote: > On 1/15/22 16:21, Martin Sebor via Libc-alpha wrote: > >> +              intptr_t ip_new_pool = (intptr_t)new_pool; >> +              intptr_t ptr_diff = ip_new_pool - ip_string_space; > > This sort of thing can have undefined behavior due to signed integer > overflow. To avoid that problem on glibc platforms, you can use > uintptr_t instead of intptr_t. Good point. But then the pointer addition in map[i].alias += ptr_diff; might overflow (that's why I used the signed intptr_t). Ugh. > >> -                  map[i].alias += new_pool - string_space; >> -                  map[i].value += new_pool - string_space; >> +                  map[i].alias += ptr_diff; >> +                  map[i].value += ptr_diff; > > Although this might pacify GCC 12, it continues to have the undefined > behavior that GCC is evidently warning about, since the old values of > map[i].alias and map[i].value have been freed and this means programs > cannot use those pointers even if only to add something to them. It's > conceivable that future GCC versions will figure this out and generate > code that doesn't do what we want here. I wondered if they might point into the allocated block. Ugh again. > > One simple workaround would be for the .alias and .value components to > be offsets into the storage pool, instead of being absolute pointers. Right, that's also the recommendation mentioned in the GCC manual. But it seems like a bigger change than I have the time to make. So I think I'll leave this mess to someone else and simply suppress the warning for this code until then. > >> --- a/stdlib/setenv.c >> +++ b/stdlib/setenv.c >> @@ -150,7 +150,9 @@ __add_to_environ (const char *name, const char >> *value, const char *combined, >>      { >>        char **new_environ; >> >> -      /* We allocated this space; we can extend it.  */ >> +      /* We allocated this space; we can extend it.  Avoid using the raw >> +     reallocated pointer to avoid GCC -Wuse-after-free.  */ >> +      uintptr_t ip_last_environ = (uintptr_t)last_environ; >>        new_environ = (char **) realloc (last_environ, >>                         (size + 2) * sizeof (char *)); >>        if (new_environ == NULL) > > Cleaner would be to leave the old code alone, except to add this before > reallocating: > >       bool we_allocated_environ = __environ == last_environ; > > > ... >> -      if (__environ != last_environ) >> +      if ((uintptr_t)__environ != ip_last_environ) > > And then change the above line to "if (! we_allocated_environ)". That does look cleaner although it wasn't entirely obvious to me from looking at the code that it's the same. Unfortunately, it doesn't help. GCC replaces the bool variable with the equality test of the two pointers, and the warning points that out. Pedantically speaking it's a bug in GCC that it does that but I doubt anyone would care to do anything about it. This only happens at level 3 (i.e., above the default 2), so if you want a clean build at that level you can either take the patch as is or use #pragma GCC diagnostic to suppress the warning. Otherwise, if all you care about is -Wall, then you can drop this part of the patch. Let me post an updated version with the changes broken up by file as Florian requested. Thanks for your comments! Martin > >>      memcpy ((char *) new_environ, (char *) __environ, >>          size * sizeof (char *)); >>