Skip to content

Commit

Permalink
Don't call av_fetch() with TRUE to create an SV that is immediately f…
Browse files Browse the repository at this point in the history
…reed.

In Perl_my_pclose() the code as been calling av_fetch() with TRUE (lvalue;
create the SV if not found) since the Perl 4 -> Perl 5 migration. The
code *had* been assuming that the returned result was always a valid SvIV
until commit 25d9202 in Jan 2001:
    Safe fix for Simon's pclose() doing SvIVX of undef -> core bug.

which fixes the bug reported in
https://www.nntp.perl.org/group/perl.perl5.porters/2001/01/msg28651.html

That commit changed the code to default the IV result (the pid) to -1 if the
av_fetch() failed to return SVt_IV. However, that commit failed to notice
that the value -1 was *already* "in use" *only 4 lines later* as a flag for
OS/2 to indicate "Opened by popen."

Hence switch the OS/2 sentinel value to -2.

The that states that OS/2 has a my_pclose implementation in os2.c is wrong.
It was erroneously added by commit 5f05dab in Dec 1996:
    [inseparable changes from patch from perl5.003_11 to perl5.003_12]

It appears to be a copy-paste error from the previous comment added about
my_popen.

I tested this fix with the 2001-era code of commit 25d9202 - it
(also) solves the bug reported back then.
  • Loading branch information
nwc10 committed Jul 28, 2021
1 parent fbc4132 commit 3235da5
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 7 deletions.
2 changes: 1 addition & 1 deletion os2/os2.c
Expand Up @@ -1666,7 +1666,7 @@ my_syspopen4(pTHX_ char *cmd, char *mode, I32 cnt, SV** args)
# endif
sv = *av_fetch(PL_fdpid, PerlIO_fileno(res), TRUE);
(void)SvUPGRADE(sv,SVt_IV);
SvIVX(sv) = -1; /* A cooky. */
SvIVX(sv) = -2; /* A cooky. */
return res;

#endif /* USE_POPEN */
Expand Down
16 changes: 10 additions & 6 deletions util.c
Expand Up @@ -3210,7 +3210,7 @@ Perl_rsignal_restore(pTHX_ int signo, Sigsave_t *save)
#endif /* !HAS_SIGACTION */
#endif /* !PERL_MICRO */

/* VMS' my_pclose() is in VMS.c; same with OS/2 */
/* VMS' my_pclose() is in VMS.c */
#if (!defined(DOSISH) || defined(HAS_FORK)) && !defined(VMS) && !defined(__LIBCATAMOUNT__) && !defined(__amigaos4__)
I32
Perl_my_pclose(pTHX_ PerlIO *ptr)
Expand All @@ -3224,10 +3224,14 @@ Perl_my_pclose(pTHX_ PerlIO *ptr)
const int fd = PerlIO_fileno(ptr);
bool should_wait;

svp = av_fetch(PL_fdpid,fd,TRUE);
pid = (SvTYPE(*svp) == SVt_IV) ? SvIVX(*svp) : -1;
SvREFCNT_dec(*svp);
*svp = NULL;
svp = av_fetch(PL_fdpid, fd, FALSE);
if (svp) {
pid = (SvTYPE(*svp) == SVt_IV) ? SvIVX(*svp) : -1;
SvREFCNT_dec(*svp);
*svp = NULL;
} else {
pid = -1;
}

#if defined(USE_PERLIO)
/* Find out whether the refcount is low enough for us to wait for the
Expand All @@ -3238,7 +3242,7 @@ Perl_my_pclose(pTHX_ PerlIO *ptr)
#endif

#ifdef OS2
if (pid == -1) { /* Opened by popen. */
if (pid == -2) { /* Opened by popen. */
return my_syspclose(ptr);
}
#endif
Expand Down

0 comments on commit 3235da5

Please sign in to comment.