Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sysopen() sets $! on success #16064

Closed
p5pRT opened this issue Jul 2, 2017 · 11 comments
Closed

sysopen() sets $! on success #16064

p5pRT opened this issue Jul 2, 2017 · 11 comments

Comments

@p5pRT
Copy link

p5pRT commented Jul 2, 2017

Migrated from rt.perl.org#131690 (status was 'rejected')

Searchable as RT131690$

@p5pRT
Copy link
Author

p5pRT commented Jul 2, 2017

From @FGasper

=================

perl -Mstrict -E'$! = 0; sysopen my $fh, "foo", 0 or die 123; say $! || "no error"'
Inappropriate ioctl for device
=================

^^ It seems $! should only be​:

- set to 0/q<> on success
- a useful value on failure

I observed the same with open() -- I tried filing on it via the perlbug script, but I think I got a “false success”​:

===================
Are you certain you want to send this message?

Please type "yes" if you are [no]​: yes
unknown​: fatal​: file /etc/postfix/main.cf​: parameter mail_owner​: user postfix has same user ID as _postfix

Sendmail returned status '1'

Thank you for taking the time to file a bug report!

Thank you for your time.

-FG

@p5pRT
Copy link
Author

p5pRT commented Jul 2, 2017

From @Leont

On Sun, Jul 2, 2017 at 5​:11 PM, felipe@​felipegasper.com <
perlbug-followup@​perl.org> wrote​:

=================

perl -Mstrict -E'$! = 0; sysopen my $fh, "foo", 0 or die 123; say $! ||
"no error"'
Inappropriate ioctl for device
=================

^^ It seems $! should only be​:

- set to 0/q<> on success
- a useful value on failure

I observed the same with open() -- I tried filing on it via the perlbug
script, but I think I got a “false success”​:

Setting $! on success is legal, it's value is only defined after a failing
call. The one thing POSIX doesn't allow for any of its functions though is
setting it to zero, and I do believe we should be following that.

I do agree setting it to some error on success is not terribly helpful
though, adding a few localizations could reduce confusion.

Leon

@p5pRT
Copy link
Author

p5pRT commented Jul 2, 2017

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2017

From @tonycoz

On Sun, 02 Jul 2017 08​:27​:50 -0700, LeonT wrote​:

Setting $! on success is legal, it's value is only defined after a failing
call. The one thing POSIX doesn't allow for any of its functions though is
setting it to zero, and I do believe we should be following that.

I do agree setting it to some error on success is not terribly helpful
though, adding a few localizations could reduce confusion.

The attached prevents the error code set that Felipe describes, but as you say, we don't promise that sysopen leaves errno alone on success, so I'm 40% inclined to just reject this ticket.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2017

From @tonycoz

0001-perl-131690-prevent-other-syscalls-modifying-errno-i.patch
From 89abca172fe8119a11a565899da1f729759b2ca5 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Wed, 5 Jul 2017 12:01:04 +1000
Subject: (perl #131690) prevent other syscalls modifying errno in sysopen

sysopen() does PerlIO initialization that can modify errno on success,
prevent that initialization from modifying errno.

This doesn't prevent the actual open(3) call from setting errno on
success.
---
 perlio.c     |  2 ++
 t/op/sysio.t | 12 ++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/perlio.c b/perlio.c
index 209af1b..5c418d0 100644
--- a/perlio.c
+++ b/perlio.c
@@ -3838,6 +3838,7 @@ PerlIOBuf_pushed(pTHX_ PerlIO *f, const char *mode, SV *arg, PerlIO_funcs *tab)
 {
     PerlIOBuf *b = PerlIOSelf(f, PerlIOBuf);
     const int fd = PerlIO_fileno(f);
+    dSAVE_ERRNO;
     if (fd >= 0 && PerlLIO_isatty(fd)) {
 	PerlIOBase(f)->flags |= PERLIO_F_LINEBUF | PERLIO_F_TTY;
     }
@@ -3847,6 +3848,7 @@ PerlIOBuf_pushed(pTHX_ PerlIO *f, const char *mode, SV *arg, PerlIO_funcs *tab)
 	    b->posn = posn;
 	}
     }
+    RESTORE_ERRNO;
     return PerlIOBase_pushed(aTHX_ f, mode, arg, tab);
 }
 
diff --git a/t/op/sysio.t b/t/op/sysio.t
index b95def0..5b399fb 100644
--- a/t/op/sysio.t
+++ b/t/op/sysio.t
@@ -6,7 +6,7 @@ BEGIN {
   set_up_inc('../lib');
 }
 
-plan tests => 48;
+plan tests => 50;
 
 open(I, 'op/sysio.t') || die "sysio.t: cannot find myself: $!";
 
@@ -233,7 +233,15 @@ is($@, '');
 close(I);
 unlink_all $outfile;
 
-chdir('..');
+SKIP: {
+    skip_if_miniperl("Fcntl requires XS", 2);
+    require Fcntl;
+    $! = 0;
+    ok(sysopen(my $fh, "TEST", Fcntl::O_RDONLY()),
+       "successfully sysopen TEST for read")
+      or skip("sysopen failed", 1);
+    is(0+$!, 0, "sysopen doesn't touch $! on success");
+}
 
 1;
 
-- 
2.1.4

@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2017

From @demerphq

On 5 Jul 2017 04​:06, "Tony Cook via RT" <perlbug-followup@​perl.org> wrote​:

On Sun, 02 Jul 2017 08​:27​:50 -0700, LeonT wrote​:

Setting $! on success is legal, it's value is only defined after a failing
call. The one thing POSIX doesn't allow for any of its functions though is
setting it to zero, and I do believe we should be following that.

I do agree setting it to some error on success is not terribly helpful
though, adding a few localizations could reduce confusion.

The attached prevents the error code set that Felipe describes, but as you
say, we don't promise that sysopen leaves errno alone on success, so I'm
40% inclined to just reject this ticket.rn

Personally I think that this goes against the design of errno. That var is
relevant if and only if a system call says so and only immediately after it
says so. Imo it is totally possible and reasonable for a system call to
internally call other system calls which may set errno elegitimately which
is then gracefully handled allowing the outer system call to return
success, at which point errno has legitimately changed.

In other words not only is it unsound to expect errno to be unchanged on
success people should assume that it *will* change on success.
.
IMO the only time we should worry about the volatility of errno is when an
invisible internal operation might clobber it before the user has had a
chance to copy it after a user level operation.

Yves

@p5pRT
Copy link
Author

p5pRT commented Jul 10, 2017

From zefram@fysh.org

Tony Cook via RT wrote​:

we don't promise that sysopen leaves errno alone on success, so I'm 40%
inclined to just reject this ticket.

Indeed, we should just reject it. errno, and therefore $!, is generally
allowed to be clobbered on success. Changing that in just one place
doesn't seem useful.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Jul 10, 2017

From cm.perl@abtela.com

Le 10/07/2017 à 22​:33, Zefram a écrit :

                           errno\, and therefore $\!\, is generally

allowed to be clobbered on success. Changing that in just one place
doesn't seem useful.

-zefram

I still do not see why $! should be so tightly anchored to errno. I
would appreciate polite perl operators (see
https://rt.perl.org/Public/Bug/Display.html?id=124232#txn-1340682).

-Christian

@p5pRT
Copy link
Author

p5pRT commented Jul 10, 2017

From zefram@fysh.org

Christian Millour wrote​:

I still do not see why $! should be so tightly anchored to errno.

That's what it always historically has been, and it's a simple
implementation. To get cleaner semantics would mean a bunch of
implementation and maintenance work, which wouldn't be worthwhile merely
for a slightly nicer $!. If we wanted to design a cleaner system,
we'd turn the syscall errors into Perl exceptions (hello autodie).

-zefram

@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2017

From @tonycoz

On Mon, 10 Jul 2017 13​:33​:47 -0700, zefram@​fysh.org wrote​:

Tony Cook via RT wrote​:

we don't promise that sysopen leaves errno alone on success, so I'm 40%
inclined to just reject this ticket.

Indeed, we should just reject it. errno, and therefore $!, is generally
allowed to be clobbered on success. Changing that in just one place
doesn't seem useful.

Based on the discussion and on that in #124232 I'm rejecting this ticket.

Tony

@p5pRT p5pRT closed this as completed Jul 17, 2017
@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2017

@tonycoz - Status changed from 'open' to 'rejected'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant