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

perldoc -f alarm suggests example code that isn't safe against race conditions #10147

Open
p5pRT opened this issue Feb 10, 2010 · 17 comments
Open

Comments

@p5pRT
Copy link

p5pRT commented Feb 10, 2010

Migrated from rt.perl.org#72666 (status was 'open')

Searchable as RT72666$

@p5pRT
Copy link
Author

p5pRT commented Feb 10, 2010

From espie@nerim.net

The example code for the use of alarm is wrong, wrong, wrong.

There is a DEADLY race condition in there, the example code is overly
optimistic​:

  eval {
  local $SIG{ALRM} = sub { die "alarm\n" }; # NB​: \n required
  alarm $timeout;
  $nread = sysread SOCKET, $buffer, $size;
alarm goes off here ->
  alarm 0;
  };
  if ($@​) {
  die unless $@​ eq "alarm\n"; # propagate unexpected errors
  # timed out
  }
  else {
  # didn't
  }

and so, we lost data from the socket. There's no way around it.
This pattern doesn't work. Use select, or threads, or anything.

OF COURSE, this doesn't happen often. Just can make things unreliable...

If you want to keep the example, the code should at least mention that it
might lose data. I see no way to fix it, though.

@p5pRT
Copy link
Author

p5pRT commented Feb 11, 2010

From @craigberry

On Tue, Feb 9, 2010 at 10​:06 PM, espie@​nerim.net
<perlbug-followup@​perl.org> wrote​:

# New Ticket Created by  espie@​nerim.net
# Please include the string​:  [perl #72666]
# in the subject line of all future correspondence about this issue.
# <URL​: http​://rt.perl.org/rt3/Ticket/Display.html?id=72666 >

The example code for the use of alarm is wrong, wrong, wrong.

There is a DEADLY race condition in there, the example code is overly
optimistic​:

      eval {
          local $SIG{ALRM} = sub { die "alarm\n" }; # NB​: \n required
          alarm $timeout;
          $nread = sysread SOCKET, $buffer, $size;
alarm goes off here ->
          alarm 0;
      };
      if ($@​) {
          die unless $@​ eq "alarm\n";   # propagate unexpected errors
          # timed out
      }
      else {
          # didn't
      }

and so, we lost data from the socket. There's no way around it.
This pattern doesn't work. Use select, or threads, or anything.

Hmm. Even though signal delivery is blocked until sysread (the Perl
op, not the syscall) completes? See the docs at​:

<http​://perldoc.perl.org/perlipc.html#Deferred-Signals-(Safe-Signals)>

@p5pRT
Copy link
Author

p5pRT commented Feb 11, 2010

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

@p5pRT
Copy link
Author

p5pRT commented Feb 11, 2010

From @ikegami

On Thu, Feb 11, 2010 at 6​:26 AM, Craig A. Berry <craig.a.berry@​gmail.com>wrote​:

On Tue, Feb 9, 2010 at 10​:06 PM, espie@​nerim.net
<perlbug-followup@​perl.org> wrote​:

# New Ticket Created by espie@​nerim.net
# Please include the string​: [perl #72666]
# in the subject line of all future correspondence about this issue.
# <URL​: http​://rt.perl.org/rt3/Ticket/Display.html?id=72666 >

The example code for the use of alarm is wrong, wrong, wrong.

There is a DEADLY race condition in there, the example code is overly
optimistic​:

  eval \{
      local $SIG\{ALRM\} = sub \{ die "alarm\\n" \}; \# NB&#8203;: \\n required
      alarm $timeout;
      $nread = sysread SOCKET\, $buffer\, $size;

alarm goes off here ->
alarm 0;
};
if ($@​) {
die unless $@​ eq "alarm\n"; # propagate unexpected errors
# timed out
}
else {
# didn't
}

and so, we lost data from the socket. There's no way around it.
This pattern doesn't work. Use select, or threads, or anything.

Hmm. Even though signal delivery is blocked until sysread (the Perl
op, not the syscall) completes? See the docs at​:

With safe signals​:

1. read system call ends
2. Perl starts copying the data into the SV*
3. Signal occurs
4. Signal is noted
5. Perl finishes copying the data into the SV*
6. sysread op ends
7. Signal handler is called.
8. Exception is thrown

No data is lost. padsv and sassign are never evaluated, so the error code is
lost. You could miss an error or eof condition, but that's it.

* -- Or whatever sysread does between the time read ends and sysread ends.

@p5pRT
Copy link
Author

p5pRT commented Feb 11, 2010

From @ikegami

On Thu, Feb 11, 2010 at 2​:30 PM, Marc Espie <espie@​nerim.net> wrote​:

Is this behavior actually documented anywhere ? This is still bad,
though,
since you lose error code and eof conditions...

I'm am going from the docs for safe signals, provided in the post above
mine.

I imagine it's quite hard to cause the problematic situation to occur.

@p5pRT
Copy link
Author

p5pRT commented Feb 12, 2010

From @craigberry

On Thu, Feb 11, 2010 at 1​:30 PM, Marc Espie <espie@​nerim.net> wrote​:

On Thu, Feb 11, 2010 at 02​:08​:49PM -0500, Eric Brine wrote​:

   With safe signals​:
   1. read system call ends
   2. Perl starts copying the data into the SV*
   3. Signal occurs
   4. Signal is noted
   5. Perl finishes copying the data into the SV*
   6. sysread op ends
   7. Signal handler is called.
   8. Exception is thrown
   No data is lost. padsv and sassign are never evaluated, so the error
   code is lost. You could miss an error or eof condition, but that's it.
   * -- Or whatever sysread does between the time read ends and sysread
   ends.

Is this behavior actually documented anywhere ?

I included a link to the docs on deferred signals earlier. Those are
written as much as possible from the point of view of a Perl
programmer, but identifying possible race conditions in the
implementation really requires digging into the code in mg.c and
elsewhere and understanding both C signals and how they are used to
implement Perl signals.

It's easy to get confused between C signal handling terms that are
used by analogy when describing signals in a Perl program and C signal
handling terms that are used when describing the implementation
details of signal handling in the Perl interpreter. That's exactly
what I did a bit earlier when I said signals were blocked for the
duration of a Perl op; that isn't quite right. They are actually
caught and thrown away after bumping a counter. (That's step 4 in
Eric's list.) From the point of view of your Perl program, it's
*like* the signal is blocked until the op completes, but it's not
actually blocked from the point of view of the C implementation.

Real blocking does come into play in step 7 of Eric's list, the
calling of the Perl signal handler. C signals are blocked while Perl
checks for any of the signals that it trapped and threw away earlier
and calls the relevant Perl signal handler. If you scan the sources
for PERL_ASYNC_CHECK(), those are the places where C signals are
blocked while it's calling the signal handler(s) provided in your Perl
program.

If you look at the main run loop in run.c​:

int
Perl_runops_standard(pTHX)
{
  dVAR;
  while ((PL_op = CALL_FPTR(PL_op->op_ppaddr)(aTHX))) {
  PERL_ASYNC_CHECK();
  }

  TAINT_NOT;
  return 0;
}

you can see that inside the loop you're either executing the op,
during which C signals are caught and deferred, or you're checking for
deferred signals and calling Perl signal handlers, during which C
signals are blocked. So the window for mayhem is pretty darn narrow.

the original example code as I read it implies that
if a timeout occurs, the sysread didn't finish, which is wrong.

Maybe, maybe not, depending on whether the syscall gets restarted when
interrupted before control returns to Perl, which may depend on
platform, specific syscall being used, and whether stdio vs. perlio is
in use. See the I/O section in the docs I referenced earlier.

(the main issue with races being that they only occur unfrequently, so
people don't notice them until they kill their critical program dead,
or make it unreliable).

Got that right. I appreciate your trying to improve things. I'm in
no way guaranteeing we're 100% safe from race conditions, though we'd
sure like to know about it if you can identify one. I'm not convinced
you have yet. A working demonstration would really clinch the deal,
though.

@p5pRT
Copy link
Author

p5pRT commented Feb 13, 2010

From espie@nerim.net

On Thu, Feb 11, 2010 at 02​:08​:49PM -0500, Eric Brine wrote​:

On Thu, Feb 11, 2010 at 6​:26 AM, Craig A. Berry
<[1]craig.a.berry@​gmail.com> wrote​:

On Tue, Feb 9, 2010 at 10​:06 PM, [2]espie@​nerim.net
<[3]perlbug-followup@​perl.org> wrote​:

# New Ticket Created by [4]espie@​nerim.net
# Please include the string​: [perl #72666]
# in the subject line of all future correspondence about this issue.
# <URL​: [5]http​://rt.perl.org/rt3/Ticket/Display.html?id=72666 >

The example code for the use of alarm is wrong, wrong, wrong.

There is a DEADLY race condition in there, the example code is overly
optimistic​:

  eval \{
      local $SIG\{ALRM\} = sub \{ die "alarm\\n" \}; \# NB&#8203;: \\n required
      alarm $timeout;
      $nread = sysread SOCKET\, $buffer\, $size;

alarm goes off here ->
alarm 0;
};
if ($@​) {
die unless $@​ eq "alarm\n"; # propagate unexpected errors
# timed out
}
else {
# didn't
}

and so, we lost data from the socket. There's no way around it.
This pattern doesn't work. Use select, or threads, or anything.

 Hmm\.  Even though signal delivery is blocked until sysread \(the Perl
 op\, not the syscall\) completes?  See the docs at&#8203;:

With safe signals​:
1. read system call ends
2. Perl starts copying the data into the SV*
3. Signal occurs
4. Signal is noted
5. Perl finishes copying the data into the SV*
6. sysread op ends
7. Signal handler is called.
8. Exception is thrown
No data is lost. padsv and sassign are never evaluated, so the error
code is lost. You could miss an error or eof condition, but that's it.
* -- Or whatever sysread does between the time read ends and sysread
ends.

Is this behavior actually documented anywhere ? This is still bad, though,
since you lose error code and eof conditions...

apart from that, the original example code as I read it implies that
if a timeout occurs, the sysread didn't finish, which is wrong.
So the example ought to be slightly larger. Namely, put some sentinel
value in $nread (undef comes to mind), then test that value within the
$@​ handler, or something.
Quick bad draft​:
  eval {
  local $SIG{ALRM} = sub { die "alarm\n" }; # NB​: \n required
  $nread = undef;
  alarm $timeout;
  $nread = sysread SOCKET, $buffer, $size;
  alarm 0;
  };
  if ($@​) {
  die unless $@​ eq "alarm\n"; # propagate unexpected errors
  # timed out
  if (defined $nread) {
  # right after the sysread finished
  oops, we lost error conditions.
  } else {
  # real time out
  }
  }
  else {
  # didn't
  }

this isn't sane.

I don't know about you, but I've fixed enough race conditions in various
3rd party programs over the years. I don't think it's reasonable to promote
insane coding styles in manpages... but that draft still is better than
the basic example.

(the main issue with races being that they only occur unfrequently, so
people don't notice them until they kill their critical program dead,
or make it unreliable).

@p5pRT
Copy link
Author

p5pRT commented Feb 13, 2010

From espie@nerim.net

On Thu, Feb 11, 2010 at 05​:26​:24AM -0600, Craig A. Berry wrote​:

On Tue, Feb 9, 2010 at 10​:06 PM, espie@​nerim.net
<perlbug-followup@​perl.org> wrote​:

# New Ticket Created by  espie@​nerim.net
# Please include the string​:  [perl #72666]
# in the subject line of all future correspondence about this issue.
# <URL​: http​://rt.perl.org/rt3/Ticket/Display.html?id=72666 >

The example code for the use of alarm is wrong, wrong, wrong.

There is a DEADLY race condition in there, the example code is overly
optimistic​:

      eval {
          local $SIG{ALRM} = sub { die "alarm\n" }; # NB​: \n required
          alarm $timeout;
          $nread = sysread SOCKET, $buffer, $size;
alarm goes off here ->
          alarm 0;
      };
      if ($@​) {
          die unless $@​ eq "alarm\n";   # propagate unexpected errors
          # timed out
      }
      else {
          # didn't
      }

and so, we lost data from the socket. There's no way around it.
This pattern doesn't work. Use select, or threads, or anything.

Hmm. Even though signal delivery is blocked until sysread (the Perl
op, not the syscall) completes? See the docs at​:

<http​://perldoc.perl.org/perlipc.html#Deferred-Signals-(Safe-Signals)>

Yes, even though. There's still a race. The window where it can go wrong is smaller,
but there's no way to make it safe.

Think about two things​:
- what does it mean to block signals during perlops ?
in $nread = sysread SOCKET, $buffer, $size;
is ALRM only blocked while sysread executes ? Does this include
stashing the result in the $nread variable ?
- even if the signal is blocked, it can still happen at the point
I say, after sysread completes, and before you disable alarm.
In which case, you will test $@​, and conclude the alarm went off,
even though the sysread completed before that.

Basically, a line such as
$nread = sysread SOCKET, $buffer, $size;
is NOT an atomic operation. It's a syscall, plus a stash operation.
If the signal occurs right after the syscall completes, and before the
stash occurs, then you lose information. Without a magic mechanism
to make this atomic with respect to signals, the code can't work
safely.

@p5pRT
Copy link
Author

p5pRT commented Feb 15, 2010

From @druud62

Marc Espie wrote​:

Quick bad draft​:
eval {
local $SIG{ALRM} = sub { die "alarm\n" }; # NB​: \n required
$nread = undef;
alarm $timeout;
$nread = sysread SOCKET, $buffer, $size;
alarm 0;
};
if ($@​) {
die unless $@​ eq "alarm\n"; # propagate unexpected errors
# timed out
if (defined $nread) {
# right after the sysread finished
oops, we lost error conditions.
} else {
# real time out
}
}
else {
# didn't
}

this isn't sane.

I dislike "eval {}; if ($@​) { ... }" examples, because $@​ is a global.

The value of eval{} itself is available​:

  eval {
  local $SIG{ALRM} = sub { die "alarm\n" }; # NB​: \n required
  $nread = undef;
  alarm $timeout;
  $nread = sysread SOCKET, $buffer, $size;
  $timeleft = alarm 0;
  1; # success
  }
  or do {
  my $eval_error = $@​ || "unknown";

  # propagate unexpected errors
  die unless $eval_error eq "alarm\n";

  # timed out
  ...
  };

--
Ruud

@p5pRT
Copy link
Author

p5pRT commented Feb 16, 2010

From @demerphq

On 15 February 2010 01​:05, Dr.Ruud <rvtol+usenet@​isolution.nl> wrote​:

Marc Espie wrote​:

Quick bad draft​:
     eval {
         local $SIG{ALRM} = sub { die "alarm\n" }; # NB​: \n required
         $nread = undef;
         alarm $timeout;
         $nread = sysread SOCKET, $buffer, $size;
         alarm 0;
     };
     if ($@​) {
         die unless $@​ eq "alarm\n";   # propagate unexpected errors
         # timed out
         if (defined $nread) {
               # right after the sysread finished
               oops, we lost error conditions.
         } else {
               # real time out
         }
     }
     else {
         # didn't
     }

this isn't sane.

I dislike "eval {}; if ($@​) { ... }" examples, because $@​ is a global.

The value of eval{} itself is available​:

     eval {
         local $SIG{ALRM} = sub { die "alarm\n" }; # NB​: \n required
         $nread = undef;
         alarm $timeout;
         $nread = sysread SOCKET, $buffer, $size;
         $timeleft = alarm 0;
         1;  # success
     }
     or do {
         my $eval_error = $@​ || "unknown";

         # propagate unexpected errors
         die unless $eval_error eq "alarm\n";

         # timed out
         ...
     };

Thanks ruud. I would have said the same thing if you hadnt.

Basically it is a categorical ERROR to test $@​ to determine if an eval failed.

You HAVE to force eval to return a non false value and check that it
did indeed so return.

Any examples we have that replicate this error should be fixed.

As far as I can tell we will never ever fix the bug that leaves us in
this state so we might as well just do our best to show the work
around as the recommended way.

cheers,
Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Feb 16, 2010

From zefram@fysh.org

demerphq wrote​:

Basically it is a categorical ERROR to test $@​ to determine if an eval failed.

You HAVE to force eval to return a non false value and check that it
did indeed so return.

It's a categorical error because a signal handler might have clobbered
$@​, right? And after you find that eval{} returned undef, how do you
find out what the exception was? Read $@​? No, that'd be an equally
categorical error, because $@​ might have been clobbered by a signal
before you read it.

If signal handlers are allowed to clobber $@​ then you can't reliably use
eval{} at all. The only solution is that signal handlers must preserve
$@​, by explicit localisation, unless they actually intend to throw
an exception. If signal handlers *are* well-behaved in this manner,
then $@​ immediately after an eval{} is perfectly usable to check both
whether an exception occurred and what that exception is.

But something conceptually similar to what you suggest is indeed how to
deal with the case at hand, of a signal interrupting a system call.

You're better off not throwing an exception from the signal handler.
Instead, the signal handler should do nothing. (Don't ignore the signal;
catch it with an empty handler.) The sysread will terminate with EINTR,
which you can pick up from $! when you find that sysread returned undef.
If sysread returned non-undef, then it completed successfully, even
if the signal handler also fired. You should pay more attention to
sysread's return value than to the operation of the signal handler.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Feb 16, 2010

From @Abigail

On Tue, Feb 16, 2010 at 11​:02​:58AM +0000, Zefram wrote​:

demerphq wrote​:

Basically it is a categorical ERROR to test $@​ to determine if an eval failed.

You HAVE to force eval to return a non false value and check that it
did indeed so return.

It's a categorical error because a signal handler might have clobbered
$@​, right? And after you find that eval{} returned undef, how do you
find out what the exception was? Read $@​? No, that'd be an equally
categorical error, because $@​ might have been clobbered by a signal
before you read it.

It's not just signal handlers. It's DESTROY as well (but that can be regarded
as a kind of signal handler).

  sub DESTROY {eval {1}}
  my $r = eval {my $x = bless [], "main"; die "Foo"; 1;} // "UNDEF";
  say "eval return value​: $r; \$\@​ = '$@​'";
  __END__
  eval return value​: UNDEF; $@​ = ''

I can't get it the other way (eval returning non-undef, with $@​ set).

If signal handlers are allowed to clobber $@​ then you can't reliably use
eval{} at all. The only solution is that signal handlers must preserve
$@​, by explicit localisation, unless they actually intend to throw
an exception. If signal handlers *are* well-behaved in this manner,
then $@​ immediately after an eval{} is perfectly usable to check both
whether an exception occurred and what that exception is.

If we think this is the right way to do, there ought to be a way to have
Perl do a 'local $@​ = $@​' before calling a signal handler or DESTROY.

But I'm sure something somewhere will break if this starts happening.

But something conceptually similar to what you suggest is indeed how to
deal with the case at hand, of a signal interrupting a system call.

You're better off not throwing an exception from the signal handler.
Instead, the signal handler should do nothing. (Don't ignore the signal;
catch it with an empty handler.) The sysread will terminate with EINTR,
which you can pick up from $! when you find that sysread returned undef.
If sysread returned non-undef, then it completed successfully, even
if the signal handler also fired. You should pay more attention to
sysread's return value than to the operation of the signal handler.

Abigail

@p5pRT
Copy link
Author

p5pRT commented Feb 16, 2010

From zefram@fysh.org

Abigail wrote​:

It's not just signal handlers. It's DESTROY as well (but that can be regarded
as a kind of signal handler).

Indeed. I've been campaigning for $@​ (and the other global status
variables) to be localised in destructors and signal handlers.

If we think this is the right way to do, there ought to be a way to have
Perl do a 'local $@​ = $@​' before calling a signal handler or DESTROY.

But I'm sure something somewhere will break if this starts happening.

Throwing an exception from a signal handler could be done intentionally
and correctly. But, as we've seen in this thread, it's difficult to
get right, and there's generally a better way. With destructors the
scope for correct use is even narrower. I'd support Perl automatically
localising the vulnerable variables, but that's a debate for another time.

In the shorter term, I'm meant to be doing a module that will let you
declare a destructor like

  DESTROY { ... }

(no "sub") and get the status variables implicitly localised. I've been
busy with other things for a while, but this is still on my agenda.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Feb 16, 2010

From @davidnicol

On Tue, Feb 16, 2010 at 6​:33 AM, Zefram <zefram@​fysh.org> wrote​:

Abigail wrote​:

It's not just signal handlers. It's DESTROY as well (but that can be regarded
as a kind of signal handler).

Indeed.  I've been campaigning for $@​ (and the other global status
variables) to be localised in destructors and signal handlers.

I believe I submitted a patch that did exactly that, or a significant
part of it, two years ago, more or less.

If we think this is the right way to do, there ought to be a way to have
Perl do a 'local $@​ = $@​' before calling a signal handler or DESTROY.

But I'm sure something somewhere will break if this starts happening.

Throwing an exception from a signal handler could be done intentionally
and correctly.  But, as we've seen in this thread, it's difficult to
get right, and there's generally a better way.  With destructors the
scope for correct use is even narrower.  I'd support Perl automatically
localising the vulnerable variables, but that's a debate for another time.

its a debate for 2007 or 2008. And the consensus taken then was to
leave things as they are.

In the shorter term, I'm meant to be doing a module that will let you
declare a destructor like

       DESTROY { ... }

(no "sub") and get the status variables implicitly localised.  I've been
busy with other things for a while, but this is still on my agenda.

-zefram

One concept that emerged from the discussion as a possible way forward
was to have $@​ be an alias for something like $^EXCEPTIONS[0] but the
mysterious handwaved part was, when does that get shifted?

Somebody somewhere may have been actually using exceptions raised
during DESTROY to communicate some important piece of information, and
we didn't want to make them switch to using some other, less
ephemeral, global variable.

http​://use.perl.org/articles/08/03/29/224206.shtml is the This Week In
Perl from that week.

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2010

From @iabyn

On Thu, Feb 11, 2010 at 06​:06​:27PM -0600, Craig A. Berry wrote​:

Got that right. I appreciate your trying to improve things. I'm in
no way guaranteeing we're 100% safe from race conditions, though we'd
sure like to know about it if you can identify one. I'm not convinced
you have yet. A working demonstration would really clinch the deal,
though.

Here's a working demo. It requires a temporary hack to pp_sysread() to
make it delay for several seconds before returning, thus giving a chance
for the C signal to go off *after* the system call returns, but *before*
perl executes the next op​:

Inline Patch
diff --git a/pp_sys.c b/pp_sys.c
index e7cdb59..f21d77e 100644
--- a/pp_sys.c
+++ b/pp_sys.c
@@ -1780,6 +1780,7 @@ PP(pp_sysread)
     /* This should not be marked tainted if the fp is marked clean */
     if (!(IoFLAGS(io) & IOf_UNTAINT))
        SvTAINTED_on(bufsv);
+    if (count==1) { int i; for (i=0; i<1000000000; i++) {} }
     SP = ORIGMARK;
     PUSHi(count);
     RETURN;

Then running the following code:

  system 'echo "ab" > /tmp/foo';
  open my $fh, '</tmp/foo' or die;
  my $buffer;

  eval {
  local $SIG{ALRM} = sub { die "alarm\n" }; # NB​: \n required
  alarm 2;
  $nread = sysread $fh, $buffer, 1;
  alarm 0;
  };
  if ($@​) {
  die unless $@​ eq "alarm\n"; # propagate unexpected errors
  print "timeout out, retrying\n";
  # timed out, try again
  $nread = sysread $fh, $buffer, 1;
  $nread == 1 or die "sysread​: $!\n";
  print "read after timeout gives [$buffer]\n";
  }
  else {
  # didn't
  print "read without timeout gives [$buffer]\n";
  }

gives as output​:

  timeout out, retrying
  read after timeout gives [b]

So, we think the sysread timed out and didn't read anything, but
when we try the sysread again, we find that the first sysread actually
read the first byte from the FH and silently disposed of it.

I rather wonder whether, now that we have safe signals (so that the "die"
isn't executed until after pp_sysread has finished), whether the following
statement in the alarm() section of perlfunc no longer holds​:

  You can't rely on the alarm causing the system call to fail with C<$!>
  set to C<EINTR> because Perl sets up signal handlers to restart system
  calls on some systems.

and thus that the correct way of doing a system call with timeout is now​:

  {
  use Errno qw(EINTR);
  local $SIG{ALRM} = sub { }; # NOOP except interrupt the syscall
  alarm $timeout;
  $nread = sysread ...;
  alarm 0;
  if (defined $nread) {
  print "got $nread bytes\n";
  }
  elsif ($! == EINTR) {
  print "sysread timed out\n";
  }
  else {
  die "sysread​: $!\n";
  }

  }
 

--
There's a traditional definition of a shyster​: a lawyer who, when the law
is against him, pounds on the facts; when the facts are against him,
pounds on the law; and when both the facts and the law are against him,
pounds on the table.
  -- Eben Moglen referring to SCO

@p5pRT
Copy link
Author

p5pRT commented Feb 20, 2013

From @jkeenan

On Fri Feb 26 13​:50​:43 2010, davem wrote​:

On Thu, Feb 11, 2010 at 06​:06​:27PM -0600, Craig A. Berry wrote​:

Got that right. I appreciate your trying to improve things. I'm in
no way guaranteeing we're 100% safe from race conditions, though we'd
sure like to know about it if you can identify one. I'm not convinced
you have yet. A working demonstration would really clinch the deal,
though.

Here's a working demo. It requires a temporary hack to pp_sysread() to
make it delay for several seconds before returning, thus giving a chance
for the C signal to go off *after* the system call returns, but *before*
perl executes the next op​:

diff --git a/pp_sys.c b/pp_sys.c
index e7cdb59..f21d77e 100644
--- a/pp_sys.c
+++ b/pp_sys.c
@​@​ -1780,6 +1780,7 @​@​ PP(pp_sysread)
/* This should not be marked tainted if the fp is marked clean */
if (!(IoFLAGS(io) & IOf_UNTAINT))
SvTAINTED_on(bufsv);
+ if (count==1) { int i; for (i=0; i<1000000000; i++) {} }
SP = ORIGMARK;
PUSHi(count);
RETURN;

Then running the following code​:

system 'echo "ab" > /tmp/foo';
open my $fh\, '\</tmp/foo' or die;
my $buffer;

eval \{
local $SIG\{ALRM\} = sub \{ die "alarm\\n" \}; \# NB&#8203;: \\n required
alarm 2;
$nread = sysread $fh\, $buffer\, 1;
alarm 0;
\};
if \($@&#8203;\) \{
die unless $@&#8203; eq "alarm\\n";   \# propagate unexpected errors
print "timeout out\, retrying\\n";
\# timed out\, try again
$nread = sysread $fh\, $buffer\, 1;
$nread == 1 or die "sysread&#8203;: $\!\\n";
print "read after timeout gives \[$buffer\]\\n";
\}
else \{
\# didn't
print "read without timeout gives \[$buffer\]\\n";
\}

gives as output​:

timeout out\, retrying
read after timeout gives \[b\]

So, we think the sysread timed out and didn't read anything, but
when we try the sysread again, we find that the first sysread actually
read the first byte from the FH and silently disposed of it.

I rather wonder whether, now that we have safe signals (so that the "die"
isn't executed until after pp_sysread has finished), whether the following
statement in the alarm() section of perlfunc no longer holds​:

You can't rely on the alarm causing the system call to fail with C\<$\!>
set to C\<EINTR> because Perl sets up signal handlers to restart system
calls on some systems\.

and thus that the correct way of doing a system call with timeout is now​:

\{
use Errno qw\(EINTR\);
local $SIG\{ALRM\} = sub \{ \}; \# NOOP except interrupt the syscall
alarm $timeout;
$nread = sysread \.\.\.;
alarm 0;
if \(defined $nread\) \{
    print "got $nread bytes\\n";
\}
elsif \($\! == EINTR\) \{
    print "sysread timed out\\n";
\}
else \{
    die "sysread&#8203;: $\!\\n";
\}

\}

I reviewed this three-year-old ticket tonight. The OP's complaint
concerned the section in pod/perlfunc.pod concerning the 'alarm'
function. ('perldoc -f alarm' will get you there.)

There was considerable back-and-forth in the ticket, but it's not clear
to me whether any of the discussants were ever convinced we had a
documentation bug. In any case, the code example in perlfunc appears
unchanged.

We could use additional eyeballs on this ticket.

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Feb 24, 2013

From @iabyn

On Tue, Feb 19, 2013 at 05​:47​:21PM -0800, James E Keenan via RT wrote​:

On Fri Feb 26 13​:50​:43 2010, davem wrote​:

Here's a working demo.

I reviewed this three-year-old ticket tonight. The OP's complaint
concerned the section in pod/perlfunc.pod concerning the 'alarm'
function. ('perldoc -f alarm' will get you there.)

There was considerable back-and-forth in the ticket, but it's not clear
to me whether any of the discussants were ever convinced we had a
documentation bug. In any case, the code example in perlfunc appears
unchanged.

We could use additional eyeballs on this ticket.

I think I demonstrated that it *was* possible to reproduce the race
condition that the OP suspected existed, and that therefore the example
code in perlfunc *could* lose data, and thus either

1 the example needs replacing with something safer if we can think of
  something;
2 perl needs to be fixed so that the race condition can't occur (probably
  not possible)
3 the example needs documenting that its not safe against race conditions.

I'm unlikely to doing further wok on this ticket for the moment.

--
Never do today what you can put off till tomorrow.

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

No branches or pull requests

2 participants