From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 66893 invoked by alias); 17 Jun 2017 08:54:16 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 64728 invoked by uid 89); 17 Jun 2017 08:54:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=no version=3.3.2 spammy= X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 17 Jun 2017 08:54:12 +0000 Received: by simark.ca (Postfix, from userid 33) id 764A71E4B1; Sat, 17 Jun 2017 04:54:15 -0400 (EDT) To: Sergio Durigan Junior Subject: Re: [PATCH v5] C++ify gdb/common/environ.c X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Sat, 17 Jun 2017 08:54:00 -0000 From: Simon Marchi Cc: GDB Patches , Pedro Alves In-Reply-To: <20170616222315.12779-1-sergiodj@redhat.com> References: <20170413040455.23996-1-sergiodj@redhat.com> <20170616222315.12779-1-sergiodj@redhat.com> Message-ID: <4e43c71a2ac4aa229bb262e18dec668c@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.5 X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg00474.txt.bz2 On 2017-06-17 00:23, Sergio Durigan Junior wrote: > void > -set_in_environ (struct gdb_environ *e, const char *var, const char > *value) > +gdb_environ::set (const char *var, const char *value) > { > - int i; > - int len = strlen (var); > - char **vector = e->vector; > - char *s; > - > - for (i = 0; (s = vector[i]) != NULL; i++) > - if (strncmp (s, var, len) == 0 && s[len] == '=') > - break; > + /* We have to unset the variable in the vector if it exists. */ > + unset (var); > > - if (s == 0) > - { > - if (i == e->allocated) > - { > - e->allocated += 10; > - vector = (char **) xrealloc ((char *) vector, > - (e->allocated + 1) * sizeof (char *)); > - e->vector = vector; > - } > - vector[i + 1] = 0; > - } > - else > - xfree (s); > - > - s = (char *) xmalloc (len + strlen (value) + 2); > - strcpy (s, var); > - strcat (s, "="); > - strcat (s, value); > - vector[i] = s; > - > - /* This used to handle setting the PATH and GNUTARGET variables > - specially. The latter has been replaced by "set gnutarget" > - (which has worked since GDB 4.11). The former affects searching > - the PATH to find SHELL, and searching the PATH to find the > - argument of "symbol-file" or "exec-file". Maybe we should have > - some kind of "set exec-path" for that. But in any event, having > - "set env" affect anything besides the inferior is a bad idea. > - What if we want to change the environment we pass to the program > - without afecting GDB's behavior? */ > - > - return; > + /* Insert the element before the last one, which is always NULL. */ > + m_environ_vector.insert (m_environ_vector.end () - 1, > + concat (var, "=", value, NULL)); The breaks if we have just constructed an empty gdb_environ object, as the vector is completely empty (no terminating NULL). So we'd need some kind of check before that, if the vector is empty, add a NULL element... I actually preferred the option of adding the NULL element to the vector in the gdb_environ constructor, since it allows always having the vector in a consistent state. I don't think that avoiding that heap allocation is worth the complexity it adds to the code (unless we can prove otherwise by memory usage profiling). > +static void > +run_tests () > +{ > + if (setenv ("GDB_SELFTEST_ENVIRON", "1", 1) != 0) > + error ("Could not set environment variable for testing."); > + > + gdb_environ env; > + > + SELF_CHECK (env.envp ()[0] == NULL); > + > + SELF_CHECK (env.get ("PWD") == NULL); If you add env.set ("PWD", "/home"); you should see a crash. Simon