Re: [PATCH v1 7/8] semihosting: clean up handling of expanded argv

[Daniel P . Berrangé]

On Tue, Mar 15, 2022 at 01:59:59PM +0000, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> writes:
> 
> > On 15/3/22 13:12, Alex Bennée wrote:
> >> Another cleanup patch tripped over the fact we weren't being careful
> >> in our casting. Fix the casts, allow for a non-const and switch from
> >> g_realloc to g_renew.
> >> The whole semihosting argument handling could do with some tests
> >> though.
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> ---
> >>   semihosting/config.c | 6 +++---
> >>   1 file changed, 3 insertions(+), 3 deletions(-)
> >> diff --git a/semihosting/config.c b/semihosting/config.c
> >> index 137171b717..50d82108e6 100644
> >> --- a/semihosting/config.c
> >> +++ b/semihosting/config.c
> >> @@ -51,7 +51,7 @@ typedef struct SemihostingConfig {
> >>       bool enabled;
> >>       SemihostingTarget target;
> >>       Chardev *chardev;
> >> -    const char **argv;
> >> +    char **argv;
> >>       int argc;
> >>       const char *cmdline; /* concatenated argv */
> >>   } SemihostingConfig;
> >> @@ -98,8 +98,8 @@ static int add_semihosting_arg(void *opaque,
> >>       if (strcmp(name, "arg") == 0) {
> >>           s->argc++;
> >>           /* one extra element as g_strjoinv() expects NULL-terminated 
> >> array */
> >> -        s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *));
> >> -        s->argv[s->argc - 1] = val;
> >> +        s->argv = g_renew(char *, s->argv, s->argc + 1);
> >> +        s->argv[s->argc - 1] = g_strdup(val);
> >
> > Why strdup()?
> 
> The compiler was having issues with adding a const char * into the array
> and it was the quickest way to stop it complaining. I'm not sure what
> guarantees you can make about a const char * after you leave the scope
> of the function.

No guarantees at all. This method was implicitly relying on the caller
never free'ing the const arg it passed in. That is indeed the case here,
because the arg came from a QemuOpts list. It is bad practice to rely
on such things though, so adding the strdup is sane IMHO.

I would have split the strdup out from the realloc -> renew change
though, since it is an independent cleanup.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|