[systemd-devel] [PATCH] core: avoid needless strdup by working with strings directly

[shawn]

On Thu, 2012-08-16 at 11:58 +0100, Colin Guthrie wrote: 
> 'Twas brillig, and Shawn Landden at 16/08/12 02:13 did gyre and gimble:
> > ---
> >  src/core/main.c |   51 ++++++++++++++++++++++-----------------------------
> >  1 file changed, 22 insertions(+), 29 deletions(-)
> > 
> > diff --git a/src/core/main.c b/src/core/main.c
> > index cdd77c1..e9b656b 100644
> > --- a/src/core/main.c
> > +++ b/src/core/main.c
> > @@ -339,12 +339,11 @@ static int parse_proc_cmdline_word(const char *word) {
> >                  else
> >                          arg_default_std_error = r;
> >          } else if (startswith(word, "systemd.setenv=")) {
> > -                char *cenv, *eq;
> > +                const char *cenv;
> > +                char *eq;
> >                  int r;
> >  
> > -                cenv = strdup(word + 15);
> > -                if (!cenv)
> > -                        return -ENOMEM;
> > +                cenv = word + 15;
> >  
> >                  eq = strchr(cenv, '=');
> >                  if (!eq) {
> > @@ -352,12 +351,12 @@ static int parse_proc_cmdline_word(const char *word) {
> >                          if (r < 0)
> >                                  log_warning("unsetenv failed %m. Ignoring.");
> >                  } else {
> > -                        *eq = 0;
> > +                        *eq = '\0';
> 
> If I am reading this correctly, you're modifying eq's value here, which
> means you're modifying word, which is a const char *. I know you change
> it back later, but if the complier puts this data in a read only page,
> you'll still get a segv so you simply cannot do this.
> 
> I've not looked through the rest of the patch, but it might be subject
> to the same issues.
No, this is the only place. I was worried about this too, and asked
someone about it, and apparently got the wrong answer. All the other's
do not have the const qualifier. (yet, entering
parse_proc_cmdline_word() casts to const)

I changed 0 to '\0' here to make it clearer the value is being changed
to insert a null byte (instead of assigning the pointer to NULL).

-- 
-Shawn Landden