Bash: sleep execution issue with bash loadable builtins

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

Bash: sleep execution issue with bash loadable builtins

Thiruvadi Rajaraman
Hi,

Found a 'sleep' execution issue with bash loadable builtins and has
performed
the below sleep test on bash-4.4-rc1.

Bash version: 4.4-rc1
Web link: https://ftp.gnu.org/gnu/bash/bash-4.4-rc1.tar.gz

Reproducible test case and Console logs:
========================================

bash-4.4-rc1#./bash
bash-4.4-rc1# cd examples/loadables/
bash-4.4-rc1/examples/loadables# enable -f ./sleep sleep

bash-4.4-rc1/examples/loadables# date; sleep 1 & date; sleep 4; date
Mon Nov 27 17:00:01 IST 2017
[1] 5355
Mon Nov 27 17:00:01 IST 2017
Mon Nov 27 17:00:02 IST 2017    --------------> Took 1 Sec only instead of
4 Sec's.
[1]+  Done                    sleep 1


bash-4.4-rc1/examples/loadables#
bash-4.4-rc1/examples/loadables# date; sleep 4 & date; sleep 1; date
Mon Nov 27 17:00:16 IST 2017
[1] 5386
Mon Nov 27 17:00:16 IST 2017
Mon Nov 27 17:00:17 IST 2017


bash-4.4-rc1/examples/loadables#
bash-4.4-rc1/examples/loadables# date; sleep 1 & date; sleep 10; date
Mon Nov 27 17:00:29 IST 2017
[1]+  Done                    sleep 4
[1] 5390
Mon Nov 27 17:00:29 IST 2017
Mon Nov 27 17:00:30 IST 2017     -------------> Took 1 Sec only instead of
10 Sec's.
[1]+  Done                    sleep 1
root@cavium-Vostro-3446
:~/Desktop/bugzilla-CGX/77504/bash-4.4-rc1/examples/loadables#

Fix patch:
==========
# Attached the fix patch -
Fix_for_bash_loadable_builtin_sleep_execution_issue.patch


Please kindly review and suggest your comments.

Thanks,
Thiruvadi Rajaraman

Fix_for_bash_loadable_builtin_sleep_execution_issue.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Bash: sleep execution issue with bash loadable builtins

Chet Ramey
On 11/27/17 4:17 AM, Thiruvadi Rajaraman wrote:
> Hi,
>
> Found a 'sleep' execution issue with bash loadable builtins and has
> performed
> the below sleep test on bash-4.4-rc1.

That's an interesting one. It looks like the SIGCHLD interrupts the select
loop, even though bash supplies SA_RESTART when installing its SIGCHLD
handler.  It's probably too hard to restart it in general, since select
doesn't necessarily modify its timeval argument when it returns early
(Linux does; many other OSs do not).

There is a problem with your fix in that, in most cases, you've just made
everything that uses this function non-interruptible, especially in an
interactive shell. I think a better fix would be to change fsleep() to cope
with select(2) being interrupted using the bash primitives that deal with
signal and trap handling.

Chet
--
``The lyf so short, the craft so long to lerne.'' - Chaucer
                 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRU    [hidden email]    http://cnswww.cns.cwru.edu/~chet/

Reply | Threaded
Open this post in threaded view
|

Re: Bash: sleep execution issue with bash loadable builtins

Ángel González
In reply to this post by Thiruvadi Rajaraman
On 2017-11-27 at 17:47 +0530, Thiruvadi Rajaraman wrote:
> Reproducible test case and Console logs:
> ========================================

Simpler test case:
bash-4.4-rc1# cd examples/loadables/
bash-4.4-rc1/examples/loadables# enable -f ./sleep sleep
bash-4.4-rc1/examples/loadables# sleep 1 & time sleep 10                        
[1] 8892

real 0m1.005s
user 0m0.001s
sys 0m0.004s
[1]+  Done                    sleep 1




> Fix patch:
> ==========
> # Attached the fix patch -
> Fix_for_bash_loadable_builtin_sleep_execution_issue.patch
>
>
> Please kindly review and suggest your comments.
>
> Thanks,
> Thiruvadi Rajaraman

I guess the line
+  } while ( (ret == -1));
should actually be while ( (ret == -1) && (errno == EINTR) );  ?


Also there's the issue that select() _may_ modify the object pointed to
by the timeout argument [POSIX]. But it may not, in which case this
would end up oversleeping.

On such system, doing eg.
 sleep 9 & time sleep 10

would end up sleeping 19 seconds.

The solution is probably to change that select() into a pselect() that
masks SIGCHLD (as well as _some_ other signals, but not SIGINT).

Best regards


Reply | Threaded
Open this post in threaded view
|

Re: Bash: sleep execution issue with bash loadable builtins

Thiruvadi Rajaraman
In reply to this post by Chet Ramey
Hi,

Thanks a lot for your review comments.

I have reworked on the bash sleep fix based on your suggestion about signal
and trap handling in fsleep( ).

I have attached the fix patch for your kind reference.

Test logs with fix patch:
==================

root@x86-generic-64:~/bash-4.2# ./bash
root@x86-generic-64:~/bash-4.2# cd examples/loadables/

root@x86-generic-64:~/bash-4.2/examples/loadables# enable -n ./sleep sleep
bash: enable: ./sleep: not a shell builtin
bash: enable: sleep: not a shell builtin
root@x86-generic-64:~/bash-4.2/examples/loadables# enable -f ./sleep sleep

root@x86-generic-64:~/bash-4.2/examples/loadables# date; sleep 1 & date; sleep
10; date
Tue Nov 28 07:18:03 UTC 2017
[1]+  Done                    sleep 1
[1] 19524
Tue Nov 28 07:18:03 UTC 2017
Tue Nov 28 07:18:13 UTC 2017
[1]+  Done                    sleep 1
root@x86-generic-64:~/bash-4.2/examples/loadables#
root@x86-generic-64:~/bash-4.2/examples/loadables#

root@x86-generic-64:~/bash-4.2/examples/loadables# date; sleep 10 & date; sleep
4; date
Tue Nov 28 07:18:25 UTC 2017
[1] 19528
Tue Nov 28 07:18:25 UTC 2017
Tue Nov 28 07:18:29 UTC 2017
root@x86-generic-64:~/bash-4.2/examples/loadables#

<--------->



Please kindly review and suggest your comments. you suggestions and
comments helps to enhance the fix
and submit the same to bash gnu mainline.

Thanks,
Thiruvadi Rajaraman


On Tue, Nov 28, 2017 at 2:09 AM, Chet Ramey <[hidden email]> wrote:

> On 11/27/17 4:17 AM, Thiruvadi Rajaraman wrote:
> > Hi,
> >
> > Found a 'sleep' execution issue with bash loadable builtins and has
> > performed
> > the below sleep test on bash-4.4-rc1.
>
> That's an interesting one. It looks like the SIGCHLD interrupts the select
> loop, even though bash supplies SA_RESTART when installing its SIGCHLD
> handler.  It's probably too hard to restart it in general, since select
> doesn't necessarily modify its timeval argument when it returns early
> (Linux does; many other OSs do not).
>
> There is a problem with your fix in that, in most cases, you've just made
> everything that uses this function non-interruptible, especially in an
> interactive shell. I think a better fix would be to change fsleep() to cope
> with select(2) being interrupted using the bash primitives that deal with
> signal and trap handling.
>
> Chet
> --
> ``The lyf so short, the craft so long to lerne.'' - Chaucer
>                  ``Ars longa, vita brevis'' - Hippocrates
> Chet Ramey, UTech, CWRU    [hidden email]    http://cnswww.cns.cwru.edu/~
> chet/
>

Fix_for_bash_loadable_builtins_sleep_issue.patch (1018 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Bash: sleep execution issue with bash loadable builtins

Chet Ramey
On 11/28/17 12:17 AM, Thiruvadi Rajaraman wrote:
> Hi,
>
> Thanks a lot for your review comments.
>
> I have reworked on the bash sleep fix based on your suggestion about signal
> and trap handling in fsleep( ).
>
> I have attached the fix patch for your kind reference.

Your patch unconditionally changes the SIGCHLD signal handler to an invalid
value (you probably meant to use SIG_IGN) without restoring it. An
interactive shell would not be able to use job control until something
internal reset the SIGCHLD handler to the correct value.

An approach that uses pselect() if available and blocks SIGCHLD for the
duration of the call, as Angel suggested, is probably the best approach.

Chet

--
``The lyf so short, the craft so long to lerne.'' - Chaucer
                 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRU    [hidden email]    http://cnswww.cns.cwru.edu/~chet/

Reply | Threaded
Open this post in threaded view
|

Re: Bash: sleep execution issue with bash loadable builtins

Chet Ramey
In reply to this post by Ángel González
On 11/27/17 2:47 PM, Ángel wrote:

> Also there's the issue that select() _may_ modify the object pointed to
> by the timeout argument [POSIX]. But it may not, in which case this
> would end up oversleeping.
>
> On such system, doing eg.
>  sleep 9 & time sleep 10
>
> would end up sleeping 19 seconds.

I can't see how, since the two sleeps would be performed by different
processes, using two separately-initialized select calls.

--
``The lyf so short, the craft so long to lerne.'' - Chaucer
                 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRU    [hidden email]    http://cnswww.cns.cwru.edu/~chet/

Reply | Threaded
Open this post in threaded view
|

Re: Bash: sleep execution issue with bash loadable builtins

Chet Ramey
On 11/28/17 9:41 AM, Chet Ramey wrote:

> On 11/27/17 2:47 PM, Ángel wrote:
>
>> Also there's the issue that select() _may_ modify the object pointed to
>> by the timeout argument [POSIX]. But it may not, in which case this
>> would end up oversleeping.
>>
>> On such system, doing eg.
>>  sleep 9 & time sleep 10
>>
>> would end up sleeping 19 seconds.
>
> I can't see how, since the two sleeps would be performed by different
> processes, using two separately-initialized select calls.

Forget this, I see it.

--
``The lyf so short, the craft so long to lerne.'' - Chaucer
                 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRU    [hidden email]    http://cnswww.cns.cwru.edu/~chet/

Reply | Threaded
Open this post in threaded view
|

Re: Bash: sleep execution issue with bash loadable builtins

Thiruvadi Rajaraman
In reply to this post by Chet Ramey
Hi Chester,

Thanks a lot for your review comments.

I reworked on the fix to solve bash sleep issue and here attached the patch.

From include/bits/signum.h,

#define SIG_DFL ((__sighandler_t) 0)            /* Default action.  */

In the attached patch fix, signal(SIGCHLD, SIG_DFL), SIG_DFL performs
the default action to ignore the interrupt signal from child which
informs the kernel that there is no user signal handler for the given
signal, and default action to ignore the signal to terminate the
program.

Though SIG_IGN ignores the given signal, faced a side effect in bash sleep like
it sleep for more seconds than the actual given seconds and it affects the
behaviour in both background and active shell sleep process.

With the attached fix, bash sleep works as expected.

Please kindly review and suggest comments.

Thanks,

Thiruvadi Rajaraman

ufuncs_bash_sleep_fix.patch (882 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Bash: sleep execution issue with bash loadable builtins

Chet Ramey
On 12/1/17 7:49 AM, Thiruvadi Rajaraman wrote:
> Hi Chester,
>
> Thanks a lot for your review comments.
>
> I reworked on the fix to solve bash sleep issue and here attached the patch.

I don't think you got my point. Why would you override bash's installed
signal handler -- disabling job and process handling -- without restoring
it before the function returns? Even if changing the signal handler were
the right thing to do, which it isn't, it's just a terrible idea to change
it unconditionally. The right fix is to block SIGCHLD during the sleep,
which you can do easily using pselect.

--
``The lyf so short, the craft so long to lerne.'' - Chaucer
                 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRU    [hidden email]    http://cnswww.cns.cwru.edu/~chet/

Reply | Threaded
Open this post in threaded view
|

Re: Bash: sleep execution issue with bash loadable builtins

Thiruvadi Rajaraman
Hi Chester,

Based on your review comments and suggestions about the earlier fixes,
reworked on the fix with pselect()
to block the signal.
Blocked the SIGCHLD signal using sigprocmask().

Attached the reworked fix patch for your kind reference.

Please kindly review the patch and suggest your comments.

Thanks,
Thiruvadi Rajaraman


>

block_SIGCHLD_in_bash_sleep.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Bash: sleep execution issue with bash loadable builtins

Eduardo Bustamante
On Mon, Dec 4, 2017 at 5:08 AM, Thiruvadi Rajaraman
<[hidden email]> wrote:
> Hi Chester,
>
> Based on your review comments and suggestions about the earlier fixes,
> reworked on the fix with pselect()
> to block the signal.

FYI,

Chet pushed a few changes related to this in commits
564452a3ec9b73a53949325cc4acb94021d5235b and
03e8e2f9c842a95b9a180097a2769ec59b082c3b, of the `devel' branch. You
might want to review them.

Reply | Threaded
Open this post in threaded view
|

Re: Bash: sleep execution issue with bash loadable builtins

Ángel González
In reply to this post by Thiruvadi Rajaraman
On 2017-12-04 at 16:38 +0530, Thiruvadi Rajaraman wrote:

> Hi Chester,
>
> Based on your review comments and suggestions about the earlier fixes,
> reworked on the fix with pselect()
> to block the signal.
> Blocked the SIGCHLD signal using sigprocmask().
>
> Attached the reworked fix patch for your kind reference.
>
> Please kindly review the patch and suggest your comments.
>
> Thanks,
> Thiruvadi Rajaraman

As you are using pselect(), you don't need those sigprocmask() calls.
They are "done inside pselect" (but atomically)

Best regards