Re: PR80613

[Trevor Saunders <tbsaunde at tbsaunde dot org>]

On Thu, May 04, 2017 at 11:52:31AM -0600, Martin Sebor wrote:
> On 05/04/2017 10:00 AM, Prathamesh Kulkarni wrote:
> > Hi,
> > As mentioned in PR, the issue is that cddce1 marks the call to
> > __builtin_strdup as necessary:
> > marking necessary through .MEM_6 stmt p_7 = __builtin_strdup (&d);
> > 
> > and since p_7 doesn't get added to worklist in propagate_necessity()
> > because it's used only within free(), it's treated as "dead"
> > and wrongly gets released.
> > The patch fixes that by adding strdup/strndup in corresponding condition

so, I think it doesn't actually matter since we can completely remove
the strdup(), but in this casewe could also replace the strdup() with
calloc(1, 1) though I'm not sure it would ever happen in practice.  The
reasoning is that accessing (&d)[] for any x other than 0 would be
invalid.  if (&d)[0] aka d is not '\0' then strdup() will access
(&d)[1].  Therefore we can infer that d is 0, and so the strdup() must
allocate one byte whose value is 0.

> > in eliminate_unnecessary_stmts().
> > 
> > Another issue, was that my previous patch failed to remove multiple
> > calls to strdup:
> > char *f(char **tt)
> > {
> >   char *t = *tt;
> >   char *p;
> > 
> >   p = __builtin_strdup (t);
> >   p = __builtin_strdup (t);
> >   return p;
> 
> Since this is clearly a bug in the program -- the first result
> leaks -- would it make sense to issue a warning before removing
> the duplicate call?  (It would be nice to issue a warning not
> just for strdup but also for other memory allocation functions,
> so perhaps that should be a separate enhancement request.)

I'm actually planning on looking at this this weekend after meaning to
for a while.  My main goal was to catch places where unique_ptr could be
used, but I think some leak detection can live in the same place.

Trev

> 
> Martin
> 
> > }
> > 
> > That's fixed in patch by adding strdup/strndup to another
> > corresponding condition in propagate_necessity() so that only one
> > instance of strdup would be kept.
> > 
> > Bootstrapped+tested on x86_64-unknown-linux-gnu.
> > Cross-testing on arm*-*-* and aarch64*-*-* in progress.
> > OK to commit if testing passes ?
> > 
> > Thanks
> > Prathamesh
> > 
>