develooper Front page | perl.perl5.porters | Postings from January 2014

Re: [perl #120591] PerlIO_tmpfile: Fall back to cwd if we have no/tmp or TMPDIR

Thread Previous
From:
Brian Fraser
Date:
January 22, 2014 02:34
Subject:
Re: [perl #120591] PerlIO_tmpfile: Fall back to cwd if we have no/tmp or TMPDIR
Message ID:
CA+nL+nZkkXP5PCPTbDgwx8ZQXpPK7UoktoYaE=+bQiuseguHSw@mail.gmail.com
On Thu, Nov 21, 2013 at 4:06 AM, bulk88 via RT <perlbug-followup@perl.org>wrote:

> On Tue Nov 19 19:22:13 2013, Hugmeir wrote:
> > This is a bug report for perl from fraserbn@gmail.com,
> > generated with the help of perlbug 1.39 running under perl 5.16.2.
> >
> >
> > -----------------------------------------------------------------
> > [Please describe your issue here]
> >
> > With this, open($fh, undef) will now work on systems without
> > a /tmp (or equivalent) where TMPDIR is not set.
> > The OS that prompted this was Android, which has no canonical temporary
> > directory;
> > the only place it has mounted as a tmpfs is /mnt/asec, but that requires
> > root-level
> > privileges.
> >
> > ---
> >  perlio.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/perlio.c b/perlio.c
> > index 60b6a59..c2ce8f6 100644
> > --- a/perlio.c
> > +++ b/perlio.c
> > @@ -5039,6 +5039,14 @@ PerlIO_tmpfile(void)
> >       /* else we try /tmp */
> >       fd = mkstemp(tempname);
> >       }
>
>
> why are you testing fd < 0 when the block directly above with "mkstemp" is
> fd < 0 conditional?
>
> > +     if (fd < 0) {
> > +         /* Try cwd */
>
> SV is already NULL due a NULL assignment earlier remove the dec, also
> SvREFCNT_dec will safely take a NULL SV, use different variant of
> SvREFCNT_dec
> > +         if ( sv )
> > +             SvREFCNT_dec(sv);
> > +         sv = newSVpvs(".");
> > +         sv_catpv(sv, tempname + 4);
> > +         fd = mkstemp(SvPVX(sv));
> > +     }
> >       if (fd >= 0) {
> >        f = PerlIO_fdopen(fd, "w+");
> >        if (f)
>
> actually, something looks funny in the existing code
> -----------------------------
>      SV * sv = NULL;
>      /*
>       * I have no idea how portable mkstemp() is ... NI-S
>       */
>      if (tmpdir && *tmpdir) {
>          /* if TMPDIR is set and not empty, we try that first */
>          sv = newSVpv(tmpdir, 0);
>          sv_catpv(sv, tempname + 4);
>          fd = mkstemp(SvPVX(sv));
>      }
>      if (fd < 0) {
>          sv = NULL; ################# didn't "sv = newSVpv(tmpdir, 0);"
> just leak?
>          /* else we try /tmp */
>          fd = mkstemp(tempname);
>      }
>

Yeah, that code was leaking. Well spotted! Fixed
in e40f8e806ef39ca48d2e94f25d2b6c5f63b4efda
Thanks for the feedback; I've merged an updated version of the patch
as b7561fc9bc645b8c7e0e9ebbb29349113cb716c1, so I'll closing this ticket.

Thread Previous


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About