Closed p5pRT closed 9 years ago
Dozens of too trusting fileno calls (and then using the returned fds for fstat etc.)\, plus two similar cases for getgroups().
Attached.
Attached.
On Sat Apr 26 12:58:05 2014\, jhi wrote:
Dozens of too trusting fileno calls (and then using the returned fds for fstat etc.)\, plus two similar cases for getgroups().
Attached.
Fails to build with -Duseithreads due to missing aTHX_ in:
if (fd \< 0) { Perl_croak("Illegal suidscript"); } else { if (PerlLIO_fstat(fd\, &PL_statbuf) \< 0) { /* may be either wrapped or real suid */ Perl_croak("Illegal suidscript"); } }
Some other issues\, the original code here for example (doio.c):
@@ -1775\,8 +1797\,9 @@ Perl_apply(pTHX_ I32 type\, SV **mark\, SV **sp) if ((gv = MAYBE_DEREF_GV(*mark))) { if (GvIO(gv) && IoIFP(GvIOp(gv))) { #ifdef HAS_FCHOWN + int fd = PerlIO_fileno(IoIFP(GvIOn(gv))); APPLY_TAINT_PROPER(); - if (fchown(PerlIO_fileno(IoIFP(GvIOn(gv)))\, val\, val2)) + if (fd >= 0 && fchown(fd\, val\, val2)) tot--; #else Perl_die(aTHX_ PL_no_func\, "fchown");
will sensibly set errno to EBADF when fchown() fails\, but the modified code doesn't.
Similarly for the others in Perl_apply()\, pp_sysread()\, pp_syswrite()\, pp_truncate()\, pp_getpeername()\, pp_stat()\, pp_fttty()\, pp_fttext() and possibly for PerlIO::mmap.
Tony
The RT System itself - Status changed from 'new' to 'open'
On Sat Apr 26 13:01:39 2014\, jhi wrote:
Attached.
--- a/doio.c +++ b/doio.c @@ -755\,7 +755\,8 @@ S_openn_cleanup(pTHX_ GV *gv\, IO *io\, PerlIO *fp\, char *mode\, const char *oname\, #if defined(HAS_FCNTL) && defined(F_SETFD) if (fd >= 0) { dSAVE_ERRNO; - fcntl(fd\,F_SETFD\,fd > PL_maxsysfd); /* can change errno */ + if (fcntl(fd\,F_SETFD\,fd > PL_maxsysfd) \< 0) /* can change errno */ + goto say_false; RESTORE_ERRNO; } #endif
Should this PerlIO_close() the handle before C\< goto say_false > ?
I suspect we should be using FD_CLOEXEC in a few other places\, but that's not made any worse by your patch.
Tony
The RT System itself - Status changed from 'new' to 'open'
On Sunday-201404-27\, 23:48\, Tony Cook via RT wrote:
On Sat Apr 26 13:01:39 2014\, jhi wrote:
Attached.
--- a/doio.c +++ b/doio.c @@ -755\,7 +755\,8 @@ S_openn_cleanup(pTHX_ GV *gv\, IO *io\, PerlIO *fp\, char *mode\, const char *oname\, #if defined(HAS_FCNTL) && defined(F_SETFD) if (fd >= 0) { dSAVE_ERRNO; - fcntl(fd\,F_SETFD\,fd > PL_maxsysfd); /* can change errno */ + if (fcntl(fd\,F_SETFD\,fd > PL_maxsysfd) \< 0) /* can change errno */ + goto say_false; RESTORE_ERRNO; } #endif
Should this PerlIO_close() the handle before C\< goto say_false > ?
Refreshed patch attached. (Also now doing the RESTORE_ERRNO before that goto say_false.)
Regarding SAVE/RESTORE_ERRNO: I am of two strongly conflicting minds...
(1) is that really worth the dance? we stomp all over it all over the place\, we rarely (if ever) do only once syscall/libcall whatever we do\, so the errno is mostly bogus anyway...
(2) we should be doing the dance in much more regimented way so that the errno would be more reliable.
And then there's of course my more blue-sky mind thinking that the return values from such things should be dual (or more) value\, so that it would be e.g. (pseudocode\, not really a hashref)
{ bool => undef\, I32 => ENOENT\, ... }
That is\, the reason for failure would be embedded in the return value. And we could obsolete $! altogether... though of course we could never deprecate it. Sigh.
I suspect we should be using FD_CLOEXEC in a few other places\, but that's not made any worse by your patch.
Yeah. The FD_CLOEXEC seems to be common thing to do whenever acquiring more fds\, it probably should be wrapped into a routine.
Tony
will sensibly set errno to EBADF when fchown() fails\, but the modified code doesn't.
Similarly for the others in Perl_apply()\, pp_sysread()\, pp_syswrite()\, pp_truncate()\, pp_getpeername()\, pp_stat()\, pp_fttty()\, pp_fttext() and possibly for PerlIO::mmap.
Yeah. Amended the patch to set EBADF when applicable.
Also now getting failures from tests\, will see what's up with them. Better hold off on this until the dust settles.
Tony
will sensibly set errno to EBADF when fchown() fails\, but the modified code doesn't.
I now explictly set errno to EBADF if fd is zero (if necessary\, some places have their own "failure goto" that sets errno)\, and return -1/undef/failure goto\, and do not even attempt the fchown() etc. That seemed saner than letting e.g. fchown() on a bad fd first happen and then setting errno to EBADF if the fd was bad.
Also now getting failures from tests\, will see what's up with them.
One stray "goto failure;" too many broke PerlIO::scalar. Now passing all tests. Refreshed patch attached.
Better hold off on this until the dust settles.
On Sat Apr 26 12:58:05 2014\, jhi wrote:
Dozens of too trusting fileno calls (and then using the returned fds for fstat etc.)\, plus two similar cases for getgroups().
Attached.
Why this isn't a horrible perf degradation?
-- bulk88 ~ bulk88 at hotmail.com
It seems this patch changes user visible behavior\, when previously true was returned\, is now undef. What are the pros and cons of this?
-- bulk88 ~ bulk88 at hotmail.com
On Wednesday-201404-30\, 1:23\, bulk88 via RT wrote:
Why this isn't a horrible perf degradation?
Huh? Before:
fileno was called it returned -1 fstat (e.g.) was called on the -1 it failed because of the bogus fd (hopefully) checked for fstat failing and returned undef/false/whatever
After: fileno was called it returned -1 we test against the -1 and if so return undef/false/whatever if we are still here\, we call fstat (e.g.) (hopefully) checked for fstat failing and returned undef/false/whatever
(Above showing only the failure paths\, not success paths.)
So it's an earlier return in case we are fed broken fds.
The only extra I see is one extra test against the returned fd. I don't think that's going to kill us.
On Wednesday-201404-30\, 1:31\, bulk88 via RT wrote:
It seems this patch changes user visible behavior\, when previously true was returned\, is now undef. What are the pros and cons of this?
Could you be more detailed? Where is the user visible behavior changed? That was not the intention. If there's a change\, there should not be.
On Wed Apr 30 03:43:33 2014\, jhi wrote:
On Wednesday-201404-30\, 1:31\, bulk88 via RT wrote:
It seems this patch changes user visible behavior\, when previously true was returned\, is now undef. What are the pros and cons of this?
Could you be more detailed? Where is the user visible behavior changed? That was not the intention. If there's a change\, there should not be.
pp_socket currently
if (!IoIFP(io) || !IoOFP(io)) { if (IoIFP(io)) PerlIO_close(IoIFP(io)); if (IoOFP(io)) PerlIO_close(IoOFP(io)); if (!IoIFP(io) && !IoOFP(io)) PerlLIO_close(fd); RETPUSHUNDEF; } #if defined(HAS_FCNTL) && defined(F_SETFD) fcntl(fd\, F_SETFD\, fd > PL_maxsysfd); /* ensure close-on-exec */ #endif
RETPUSHYES; } #endif
Your patch is
@@ -2400\,7 +2402\,8 @@ PP(pp_socket) RETPUSHUNDEF; } #if defined(HAS_FCNTL) && defined(F_SETFD) - fcntl(fd\, F_SETFD\, fd > PL_maxsysfd); /* ensure close-on-exec */ + if (fcntl(fd\, F_SETFD\, fd > PL_maxsysfd) \< 0) /* ensure close-on-exec */ + RETPUSHUNDEF; #endif
Previously\, if fnctl executed\, regardless if it failed\, the functioned returned SV * YES/true. Now it might in some rare condition (IDK what it would be\, can someone give an example why PerlIO_fdopen would succeed but fnctl fail later?) return undef. On the otherhand\, P5P has failed to documented the return value of socket() since forever http://perl5.git.perl.org/perl.git/blob/HEAD:/pod/perlfunc.pod#l6569 (I'd file a bug but dont have time to go hunting for all other retval not doced in perlfunc cases ATM).
I picked pp_socket above since it was the easiest one to write up.
Also\, I'm not a *nix person\, why do we call F_SETFD with "fd > PL_maxsysfd" instead of a proper C constant\, optimization? will the constant always be 1?
-- bulk88 ~ bulk88 at hotmail.com
On Wednesday-201404-30\, 16:06\, bulk88 via RT wrote:
Previously\, if fnctl executed\, regardless if it failed\, the functioned returned SV * YES/true. Now it might in some rare condition (IDK what it would be\, can someone give an example why PerlIO_fdopen would succeed but fnctl fail later?) return undef. On the otherhand\, P5P has failed to documented the return value of socket() since foreverhttp://perl5.git.perl.org/perl.git/blob/HEAD:/pod/perlfunc.pod#l6569 (I'd file a bug but dont have time to go hunting for all other retval not doced in perlfunc cases ATM).
Ahh\, I see what you mean. Well\, this is kind of fuzzy area... if you look just before the fcntl dance\, if the fps were dubious (either we internally messed up something\, or if the app messed up its logic\, it e.g. closed the handle)\, we did (and do) return undef.
So I extended that "if the file handle is dubious" logic to include also the fcntl failure. (Here and elsewhere in the patch.)
Also\, I'm not a *nix person\, why do we call F_SETFD with "fd > PL_maxsysfd" instead of a proper C constant\, optimization? will the constant always be 1?
The PL_maxsysfd is initialized from #define MAXSYSFD which is by default always two (covering stdin\, stdout\, stderr). So whether we F_SETFD depends on whether the fd is beyond those. I think *officially* we should be using for the third argument the FD_CLOEXEC (which is one) for true (close-on-exec).
FWIW\, we should probably use the O_CLOEXEC flag where available\, when getting the fd in the first place. I think it's only defined for open()\, not for e.g. socket() etc. which limits its usefulness.
On Wed\, Apr 30\, 2014 at 10:47 PM\, Jarkko Hietaniemi \jhi@​iki\.fi wrote:
Also\, I'm not a *nix person\, why do we call F_SETFD with "fd > PL_maxsysfd" instead of a proper C constant\, optimization? will the constant always be 1?
The PL_maxsysfd is initialized from #define MAXSYSFD which is by default always two (covering stdin\, stdout\, stderr). So whether we F_SETFD depends on whether the fd is beyond those. I think *officially* we should be using for the third argument the FD_CLOEXEC (which is one) for true (close-on-exec).
Technically yes\, though I share the impression it's universally 1.
FWIW\, we should probably use the O_CLOEXEC flag where available\, when getting the fd in the first place. I think it's only defined for open()\, not for e.g. socket() etc. which limits its usefulness.
AFAIK that's a Linuxism (though BSDs seem to be implementing it too now)\, but it's definitely a good idea in multi-threaded situations (we have to keep the fcntl to un-cloexec if fd \< PL_maxsysfd though). And actually\, it is available for sockets by or-ing the socket type argument with SOCK_CLOEXEC.
Leon
On Wednesday-201404-30\, 17:57\, Leon Timmermans wrote:
AFAIK that's a Linuxism (though BSDs seem to be implementing it too now)
It's Official\, don't know exactly since when:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
Yet again refreshed patch\, found two more spots with the same potentially negative fd use.
On Thursday-201405-01\, 17:05\, Jarkko Hietaniemi wrote:
Yet again refreshed patch\, found two more spots with the same potentially negative fd use.
And one more spot added (missed that a new Coverity scan had found more). Also hopefully better "summary line" now.
On Thursday-201405-01\, 21:06\, Jarkko Hietaniemi wrote:
On Thursday-201405-01\, 17:05\, Jarkko Hietaniemi wrote:
Yet again refreshed patch\, found two more spots with the same potentially negative fd use.
And one more spot added (missed that a new Coverity scan had found more). Also hopefully better "summary line" now.
Argh. Please discard this very latest patch ("one more spot")\, sorry about that. I'm getting mixed up in my patches\, too many in-flight. The one with "two more spots" is the currently correct one for this issue (not checking for negative return values).
Updated patch attached.
On Thursday-201405-01\, 17:05\, Jarkko Hietaniemi wrote:
Yet again refreshed patch\, found two more spots with the same potentially negative fd use.
I decided to merge this ticket with perl #121745 which checked for fcntl failure paths\, since there's a lot of functional overlap: (1) a fd from fileno being checked against \< 0\, and then (2) the fd being fed to fcntl\, the return value of which we want to check for failure (3) in one actual case of a merge conflict (in perl.c) because the changes for these two checks were too close for comfort
So updated combined patch attached\, and please ignore/merge #121745.
On Friday-201405-02\, 22:27\, Jarkko Hietaniemi wrote:
On Thursday-201405-01\, 17:05\, Jarkko Hietaniemi wrote:
Yet again refreshed patch\, found two more spots with the same potentially negative fd use.
I decided to merge this ticket with perl #121745 which checked for fcntl failure paths\, since there's a lot of functional overlap: (1) a fd from fileno being checked against \< 0\, and then (2) the fd being fed to fcntl\, the return value of which we want to check for failure (3) in one actual case of a merge conflict (in perl.c) because the changes for these two checks were too close for comfort
So updated combined patch attached\, and please ignore/merge #121745.
... and refreshed. With a fresh cup of coffee found an embarrassing typo of fd\<0 when fd>=0 was meant (on a memory-alloc-failed path: in general we can't easily test those...)
On Saturday-201405-03\, 8:30\, Jarkko Hietaniemi wrote:
On Friday-201405-02\, 22:27\, Jarkko Hietaniemi wrote:
On Thursday-201405-01\, 17:05\, Jarkko Hietaniemi wrote:
Yet again refreshed patch\, found two more spots with the same potentially negative fd use.
I decided to merge this ticket with perl #121745 which checked for fcntl failure paths\, since there's a lot of functional overlap: (1) a fd from fileno being checked against \< 0\, and then (2) the fd being fed to fcntl\, the return value of which we want to check for failure (3) in one actual case of a merge conflict (in perl.c) because the changes for these two checks were too close for comfort
So updated combined patch attached\, and please ignore/merge #121745.
... and refreshed. With a fresh cup of coffee found an embarrassing typo of fd\<0 when fd>=0 was meant (on a memory-alloc-failed path: in general we can't easily test those...)
..and yet again\, to fix a failure in t/porting/diag.
On Thu May 01 18:16:28 2014\, jhi wrote:
Updated patch attached.
It's a little inconsistent about cleaning up file handles that have failed the fcntl() call:
--- a/doio.c +++ b/doio.c @@ -755\,8 +755\,12 @@ S_openn_cleanup(pTHX_ GV *gv\, IO *io\, PerlIO *fp\, char *mode\, const char *oname\, #if defined(HAS_FCNTL) && defined(F_SETFD) if (fd >= 0) { dSAVE_ERRNO; - fcntl(fd\,F_SETFD\,fd > PL_maxsysfd); /* can change errno */ + int rc = fcntl(fd\,F_SETFD\,fd > PL_maxsysfd); /* can change errno */ RESTORE_ERRNO; + if (rc \< 0) { + PerlLIO_close(fd); + goto say_false; + } } #endif IoIFP(io) = fp;
closes the handle (but the errno from fcntl() failing is lost)
--- a/pp_sys.c +++ b/pp_sys.c @@ -715\,8 +715\,10 @@ PP(pp_pipe_op) goto badexit; } #if defined(HAS_FCNTL) && defined(F_SETFD) - fcntl(fd[0]\,F_SETFD\,fd[0] > PL_maxsysfd); /* ensure close-on-exec */ - fcntl(fd[1]\,F_SETFD\,fd[1] > PL_maxsysfd); /* ensure close-on-exec */ + /* ensure close-on-exec */ + if ((fcntl(fd[0]\, F_SETFD\,fd[0] > PL_maxsysfd) \< 0) || + (fcntl(fd[1]\, F_SETFD\,fd[1] > PL_maxsysfd) \< 0)) + goto badexit; #endif RETPUSHYES;
@@ -2400\,7 +2402\,8 @@ PP(pp_socket) RETPUSHUNDEF; } #if defined(HAS_FCNTL) && defined(F_SETFD) - fcntl(fd\, F_SETFD\, fd > PL_maxsysfd); /* ensure close-on-exec */ + if (fcntl(fd\, F_SETFD\, fd > PL_maxsysfd) \< 0) /* ensure close-on-exec */ + RETPUSHUNDEF; #endif
RETPUSHYES; @@ -2445\,8 +2448\,10 @@ PP(pp_sockpair) RETPUSHUNDEF; } #if defined(HAS_FCNTL) && defined(F_SETFD) - fcntl(fd[0]\,F_SETFD\,fd[0] > PL_maxsysfd); /* ensure close-on-exec */ - fcntl(fd[1]\,F_SETFD\,fd[1] > PL_maxsysfd); /* ensure close-on-exec */ + /* ensure close-on-exec */ + if ((fcntl(fd[0]\,F_SETFD\,fd[0] > PL_maxsysfd) \< 0) || + (fcntl(fd[1]\,F_SETFD\,fd[1] > PL_maxsysfd) \< 0)) + RETPUSHUNDEF; #endif
RETPUSHYES; (and a few more)
leaks the handle(s).
Though I wonder whether a handle that fails fcntl(F_SETFD) will successfully close.
Tony
On Sun May 04 22:00:20 2014\, tonyc wrote:
On Thu May 01 18:16:28 2014\, jhi wrote:
Updated patch attached.
It's a little inconsistent about cleaning up file handles that have failed the fcntl() call:
Oops\, 121743 asked me to ignore 121745\, I'll merge as you suggested.
Tony
On Sun May 04 16:43:43 2014\, jhi wrote:
On Saturday-201405-03\, 8:30\, Jarkko Hietaniemi wrote:
On Friday-201405-02\, 22:27\, Jarkko Hietaniemi wrote:
On Thursday-201405-01\, 17:05\, Jarkko Hietaniemi wrote:
Yet again refreshed patch\, found two more spots with the same potentially negative fd use.
I decided to merge this ticket with perl #121745 which checked for fcntl failure paths\, since there's a lot of functional overlap: (1) a fd from fileno being checked against \< 0\, and then (2) the fd being fed to fcntl\, the return value of which we want to check for failure (3) in one actual case of a merge conflict (in perl.c) because the changes for these two checks were too close for comfort
So updated combined patch attached\, and please ignore/merge #121745.
... and refreshed. With a fresh cup of coffee found an embarrassing typo of fd\<0 when fd>=0 was meant (on a memory-alloc-failed path: in general we can't easily test those...)
..and yet again\, to fix a failure in t/porting/diag.
--- a/dist/IO/IO.xs +++ b/dist/IO/IO.xs @@ -524\,9 +524\,15 @@ fsync(arg) handle = IoOFP(sv_2io(arg)); if (!handle) handle = IoIFP(sv_2io(arg)); - if(handle) - RETVAL = fsync(PerlIO_fileno(handle)); - else { + if (handle) { + int fd = PerlIO_fileno(handle); + if (fd >= 0) { + RETVAL = fsync(fd); + } else { + RETVAL = -1; + errno = EINVAL; + } + } else { RETVAL = -1; errno = EINVAL; }
Elsewhere you use EBADF when fd is negative.
@@ -1616\,7 +1618\,7 @@ PP(pp_sysread) char *buffer; STRLEN orig_size; SSize_t length; - SSize_t count; + SSize_t count = -1; SV *bufsv; STRLEN blen; int fp_utf8; ... @@ -1771\,8 +1778\,11 @@ PP(pp_sysread) else #endif { - count = PerlLIO_read(PerlIO_fileno(IoIFP(io))\, - buffer\, length); + int fd = PerlIO_fileno(IoIFP(io)); + if (fd \< 0) + SETERRNO(EBADF\,RMS_IFI); + else + count = PerlLIO_read(fd\, buffer\, length); } } else
Shouldn't this explicitly set count rather than relying upon the initialization at the top?
eg. if we're reading from a PerlIO_isutf8() STDIN and another thread closes it:
T1: successful partial read (partial utf-8 character) T2: close STDIN T1: fd is negative (because it's closed) but count is still positive
(Yes\, sysread() handling on UTF-8 streams is crazy.)
@@ -2224\,13 +2237\,18 @@ PP(pp_truncate) result = 0; } else { - PerlIO_flush(fp); + int fd = PerlIO_fileno(fp); + if (fd \< 0) + SETERRNO(EBADF\,RMS_IFI); + else { + PerlIO_flush(fp); #ifdef HAS_TRUNCATE - if (ftruncate(PerlIO_fileno(fp)\, len) \< 0) + if (ftruncate(fd\, len) \< 0) #else - if (my_chsize(PerlIO_fileno(fp)\, len) \< 0) + if (my_chsize(fd\, len) \< 0) #endif - result = 0; + result = 0; + } } } }
result isn't zeroed if fd is negative.
I haven't finished working my way through this.
Tony
On Monday-201405-05\, 2:56\, Tony Cook via RT wrote:
+++ b/dist/IO/IO.xs Elsewhere you use EBADF when fd is negative.
Yes\, I remember this spot... I did use EINVAL for consistency with the existing logic *at this spot*: if there was no file pointer\, it used EINVAL. But of course EBADF would be more consistent with the rest of the change. I dunno. Which "failure contour" to follow?
@@ -1616\,7 +1618\,7 @@ PP(pp_sysread) + SSize_t count = -1; Shouldn't this explicitly set count rather than relying upon the initialization at the top?
I think I did the count init not so much because of the is-fd-negative logic but because I think I saw a possible code path where count was left uninitialized. But can't see it now\, so recanted that init.
(Though\, in principle\, initializing a variable to an illegal value as opposed to uninitialized should not cause *more* failures\, if they do\, there's something rotten with the logic...)
While I was looking at pp_sysread + pp_syswrite I cleaned up some further logic.
@@ -2224\,13 +2237\,18 @@ PP(pp_truncate)
result isn't zeroed if fd is negative.
Fixed.
Refreshed patch attached.
Refreshed patch attached.
Sigh. This is getting annoying. Too many damn code paths. One more spot fixed\, in perl.c (if we already did fd=fileno\, let's use the fd...) Attached.
(\<old_grumpy_man>And running full "make test" is too damn slow these days.\</old_grumpy_man>)
On Mon\, May 5\, 2014 at 3:13 PM\, Jarkko Hietaniemi \jhi@​iki\.fi wrote:
Refreshed patch attached.
Sigh. This is getting annoying. Too many damn code paths. One more spot fixed\, in perl.c (if we already did fd=fileno\, let's use the fd...) Attached.
(\<old_grumpy_man>And running full "make test" is too damn slow these days.\</old_grumpy_man>)
TEST_JOBS=30 make test_harness -j30
:D
On Monday-201405-05\, 9:38\, Brian Fraser via RT wrote:
TEST_JOBS=30 make test_harness -j30
Dude\, can you spare some cores?
On Mon\, 05 May 2014 09:39:58 -0400\, Jarkko Hietaniemi \jhi@​iki\.fi wrote:
On Monday-201405-05\, 9:38\, Brian Fraser via RT wrote:
TEST_JOBS=30 make test_harness -j30
Dude\, can you spare some cores?
Would access to
Linux 2.6.32-358.el6.x86_64/#1 x86_64 Xeon(R) CPU L5640 @ 2.27GHz/2267(24) x86_64 96729 Mb
Help you gain more speed in your work?
-- H.Merijn Brand http://tux.nl Perl Monger http://amsterdam.pm.org/ using perl5.00307 .. 5.19 porting perl5 on HP-UX\, AIX\, and openSUSE http://mirrors.develooper.com/hpux/ http://www.test-smoke.org/ http://qa.perl.org http://www.goldmark.org/jeff/stupid-disclaimers/
On Monday-201405-05\, 9:46\, H. Merijn Brand via RT wrote:
Linux 2.6.32-358.el6.x86_64/#1 x86_64 Xeon(R) CPU L5640 @ 2.27GHz/2267(24) x86_64 96729 Mb Help you gain more speed in your work?
It would be certainly faster than my five-year old MacBook Pro :-)
On 5 May 2014 15:53\, Jarkko Hietaniemi \jhi@​iki\.fi wrote:
On Monday-201405-05\, 9:46\, H. Merijn Brand via RT wrote:
Linux 2.6.32-358.el6.x86_64/#1 x86_64 Xeon(R) CPU L5640 @ 2.27GHz/2267(24) x86_64 96729 Mb
Help you gain more speed in your work?
It would be certainly faster than my five-year old MacBook Pro :-)
We will fix that then.
Yves
-- perl -Mre=debug -e "/just|another|perl|hacker/"
On ma\, 2014-05-05 at 16:07 +0200\, demerphq wrote:
On 5 May 2014 15:53\, Jarkko Hietaniemi \jhi@​iki\.fi wrote: On Monday-201405-05\, 9:46\, H. Merijn Brand via RT wrote: Linux 2.6.32-358.el6.x86_64/#1 x86_64 Xeon(R) CPU L5640 @ 2.27GHz/2267(24) x86_64 96729 Mb
Help you gain more speed in your work? It would be certainly faster than my five-year old MacBook Pro :-)
We will fix that then.
The power of delegation:
[dkaarsemaker@dromedary-001 ~]$ id jhi uid=1156(jhi) gid=1003(p5p) groups=1003(p5p)
:)
-- Dennis Kaarsemaker http://www.kaarsemaker.net
On Mon May 05 05:33:07 2014\, jhi wrote:
On Monday-201405-05\, 2:56\, Tony Cook via RT wrote:
+++ b/dist/IO/IO.xs Elsewhere you use EBADF when fd is negative.
Yes\, I remember this spot... I did use EINVAL for consistency with the existing logic *at this spot*: if there was no file pointer\, it used EINVAL. But of course EBADF would be more consistent with the rest of the change. I dunno. Which "failure contour" to follow?
I'm aiming for the old behaviour - fsync() etc would have been setting errno=EBADF when supplied with a bad file handle.
@@ -1616\,7 +1618\,7 @@ PP(pp_sysread) + SSize_t count = -1; Shouldn't this explicitly set count rather than relying upon the initialization at the top?
I think I did the count init not so much because of the is-fd-negative logic but because I think I saw a possible code path where count was left uninitialized. But can't see it now\, so recanted that init.
(Though\, in principle\, initializing a variable to an illegal value as opposed to uninitialized should not cause *more* failures\, if they do\, there's something rotten with the logic...)
While I was looking at pp_sysread + pp_syswrite I cleaned up some further logic.
I wasn't clear enough here\, this code:
@@ -1765\,14 +1775\,18 @@ PP(pp_sysread) if (PL_op->op_type == OP_SYSREAD) { #ifdef PERL_SOCK_SYSREAD_IS_RECV if (IoTYPE(io) == IoTYPE_SOCKET) { - count = PerlSock_recv(PerlIO_fileno(IoIFP(io))\, - buffer\, length\, 0); + if (fd \< 0) + SETERRNO(EBADF\,SS_IVCHAN); + else + count = PerlSock_recv(fd\, length\, 0); } else #endif { - count = PerlLIO_read(PerlIO_fileno(IoIFP(io))\, - buffer\, length); + if (fd \< 0) + SETERRNO(EBADF\,RMS_IFI); + else + count = PerlLIO_read(fd\, buffer\, length); } } else
doesn't set count when fd is negative\, probably breaking the code that follows that checks and uses count.
With the initialization\, the first time around the loop would have been safe\, but on a UTF-8 stream\, sysread() can loop to fill out partial UTF-8 sequences\, which would have left count as the previous value - I think fd could change if we were reading from STDIN and another thread closed it.
@@ -696\,7 +696\,10 @@ S_openn_cleanup(pTHX_ GV *gv\, IO *io\, PerlIO *fp\, char *mode\, const char *oname\, is assigned to (say) STDOUT - for now let dup2() fail and provide the error */ - if (PerlLIO_dup2(fd\, savefd) \< 0) { + if (fd \< 0) { + SETERRNO(EBADF\,RMS_IFI); + goto say_false; + } else if (PerlLIO_dup2(fd\, savefd) \< 0) { (void)PerlIO_close(fp); goto say_false; }
The comment you can see the tail of here is now incorrect\, I think.
@@ -755\,8 +768\,12 @@ S_openn_cleanup(pTHX_ GV *gv\, IO *io\, PerlIO *fp\, char *mode\, const char *oname\, #if defined(HAS_FCNTL) && defined(F_SETFD) if (fd >= 0) { dSAVE_ERRNO; - fcntl(fd\,F_SETFD\,fd > PL_maxsysfd); /* can change errno */ + int rc = fcntl(fd\,F_SETFD\,fd > PL_maxsysfd); /* can change errno */ RESTORE_ERRNO; + if (rc \< 0) { + PerlLIO_close(fd); + goto say_false; + } } #endif IoIFP(io) = fp;
If we're failing the open() based on fcntl() failing\, the errno from that failure should be visible to the caller. I think that means we can remove the dSAVE_ERRNO/RESTORE_ERRNO pair.
Sigh. This is getting annoying. Too many damn code paths. One more spot fixed\, in perl.c (if we already did fd=fileno\, let's use the fd...) Attached.
Comments based on this patch.
Tony
EINVAL. But of course EBADF would be more consistent with the rest of the change. I dunno. Which "failure contour" to follow?
I'm aiming for the old behaviour - fsync() etc would have been setting errno=EBADF when supplied with a bad file handle.
Ah\, I see. With that in mind\, IO.xs given a new shake.
I wasn't clear enough here\, this code:
@@ -1765\,14 +1775\,18 @@ PP(pp_sysread) if (PL_op->op_type == OP_SYSREAD) { ... + if (fd \< 0) + SETERRNO(EBADF\,RMS_IFI); + else + count = PerlLIO_read(fd\, buffer\, length); } } else
doesn't set count when fd is negative\, probably breaking the code that follows that checks and uses count.
Now see. Now setting the count to -1 on the failure branches.
in a UTF-8 stream\, sysread() can loop to fill out partial UTF-8 sequences\, which would have left count as the previous value
Years of therapy... all wasted.
- I think fd could change if we were reading from STDIN and another thread closed it.
Aaaaaa. Hmmmm... maybe reestablishing fd from fileno at more_bytes label would help for this?
(Not attaching refreshed patch until we hash this one out.)
@@ -755\,8 +768\,12 @@ S_openn_cleanup(pTHX_ GV *gv\, IO *io\, PerlIO *fp\, char *mode\, const char *oname\, #if defined(HAS_FCNTL) && defined(F_SETFD) if (fd >= 0) { dSAVE_ERRNO; - fcntl(fd\,F_SETFD\,fd > PL_maxsysfd); /* can change errno */ + int rc = fcntl(fd\,F_SETFD\,fd > PL_maxsysfd); /* can change errno */ RESTORE_ERRNO; + if (rc \< 0) { + PerlLIO_close(fd); + goto say_false; + } } #endif IoIFP(io) = fp;
If we're failing the open() based on fcntl() failing\, the errno from that failure should be visible to the caller. I think that means we can remove the dSAVE_ERRNO/RESTORE_ERRNO pair.
Gone.
Oh\, well. Went ahead and refreshed patch anyway.
Migrated from rt.perl.org#121743 (status was 'resolved')
Searchable as RT121743$