[PATCH] fix problem with uconvert

[izabera]

if the argument is in the (-1, 0) range, the integer part is zero and
multiplying it by -1 has no effect, so the caller can't tell that the
argument was negative
---
 builtins/read.def  | 10 +-
 examples/loadables/sleep.c |  4 ++--
 externs.h  |  2 +-
 lib/sh/uconvert.c  |  7 ---
 4 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/builtins/read.def b/builtins/read.def
index 3c38bc02..fb4f9ac7 100644
--- a/builtins/read.def
+++ b/builtins/read.def
@@ -208,7 +208,7 @@ read_builtin (list)
   int lastsig, t_errno;
   int mb_cur_max;
   unsigned int tmsec, tmusec;
-  long ival, uval;
+  long ival, uval, sign;
   intmax_t intval;
   char c;
   char *input_string, *orig_input_string, *ifs_chars, *prompt, *arrayname;
@@ -302,8 +302,8 @@ read_builtin (list)
  break;
 #endif
case 't':
- code = uconvert (list_optarg, &ival, &uval, (char **)NULL);
- if (code == 0 || ival < 0 || uval < 0)
+ code = uconvert (list_optarg, &sign, &ival, &uval, (char **)NULL);
+ if (code == 0 || sign < 0 || uval < 0)
{
  builtin_error (_("%s: invalid timeout specification"), 
list_optarg);
  return (EXECUTION_FAILURE);
@@ -403,8 +403,8 @@ read_builtin (list)
   /* $TMOUT, if set, is the default timeout for read. */
   if (have_timeout == 0 && (e = get_string_value ("TMOUT")))
 {
-  code = uconvert (e, &ival, &uval, (char **)NULL);
-  if (code == 0 || ival < 0 || uval < 0)
+  code = uconvert (e, &sign, &ival, &uval, (char **)NULL);
+  if (code == 0 || sign < 0 || uval < 0)
tmsec = tmusec = 0;
   else
{
diff --git a/examples/loadables/sleep.c b/examples/loadables/sleep.c
index 204601f5..971874f0 100644
--- a/examples/loadables/sleep.c
+++ b/examples/loadables/sleep.c
@@ -58,7 +58,7 @@ parse_gnutimefmt (char *string, long *sp, long *up)
 {
int c, r;
char*s, *ep;
-   longtsec, tusec, accumsec, accumusec, t;
+   longtsec, tusec, accumsec, accumusec, t, sign;
int mult;
 
tsec = tusec = 0;
@@ -66,7 +66,7 @@ parse_gnutimefmt (char *string, long *sp, long *up)
mult = 1;
 
for (s = string; s && *s; s++) {
-   r = uconvert(s, &accumsec, &accumusec, &ep);
+   r = uconvert(s, &sign, &accumsec, &accumusec, &ep);
if (r == 0 && *ep == 0)
return r;
c = *ep;
diff --git a/externs.h b/externs.h
index 931dba9c..4030e39a 100644
--- a/externs.h
+++ b/externs.h
@@ -494,7 +494,7 @@ extern int sh_mktmpfd PARAMS((char *, int, char **));
 extern char *sh_mktmpdir PARAMS((char *, int));
 
 /* declarations for functions defined in lib/sh/uconvert.c */
-extern int uconvert PARAMS((char *, long *, long *, char **));
+extern int uconvert PARAMS((char *, long *, long *, long *, char **));
 
 /* declarations for functions defined in lib/sh/ufuncs.c */
 extern unsigned int falarm PARAMS((unsigned int, unsigned int));
diff --git a/lib/sh/uconvert.c b/lib/sh/uconvert.c
index 457552eb..c11d16c0 100644
--- a/lib/sh/uconvert.c
+++ b/lib/sh/uconvert.c
@@ -39,7 +39,8 @@
 
 #define RETURN(x) \
 do { \
-  if (ip) *ip = ipart * mult; \
+  if (sp) *sp = mult; \
+  if (ip) *ip = ipart; \
   if (up) *up = upart; \
   if (ep) *ep = p; \
   return (x); \
@@ -57,9 +58,9 @@ static int multiplier[7] = { 1, 10, 1, 1000, 100, 10, 
1 };
Return 1 if value converted; 0 if invalid integer for either whole or
fractional parts. */
 int
-uconvert(s, ip, up, ep)
+uconvert(s, sp, ip, up, ep)
  char *s;
- long *ip, *up;
+ long *sp, *ip, *up;
  char **ep;
 {
   int n, mult;
-- 
2.34.1

wait inside subshell waits for sibling

[Oğuz İsmail Uysal]

To reproduce:

   $ echo $BASH_VERSION
   5.2.2(3)-release
   $ ( : & wait ) > >(cat)
   *hangs*

It should return immediately, but hangs instead.

Feature request - make type show that a builtin is a loadable

[Geir Hauge]

I think it's a good idea for the type builtin to distinguish between
static and loadable builtins, and for debugging scripts that use
loadable builtins, it would be useful to be able to see which file got
loaded. For example in cases where BASH_LOADABLES_PATH was
unintentionally unset or set to a wrong path.

I had a go at implementing it and cobbled together the attached patch.
It extends struct builtin with a new path field that gets set to the
path that was passed to dlopen. The type builtin then includes that path
in its output if it is set.

$ ./bash -c 'enable csv; type [ csv'
[ is a shell builtin
csv is a shell builtin (/usr/local/lib/bash/csv)
$ BASH_LOADABLES_PATH=$PWD/examples/loadables ./bash -c 'enable csv; type [ 
csv'
[ is a shell builtin
csv is a shell builtin (/Users/geirha/bash/examples/loadables/csv)

Continuing on the debugging aspect, I think it would also be useful if
enable would warn or fail if you try to enable a loadable builtin that
was built against a different bash version. The easiest is probably to
embed the bash version inside the loadable file somehow, and compare
that when loading. Giving something like:

$ enable csv
bash: enable: /usr/local/lib/bash/csv: builtin was built for bash-5.1.16

Not sure how to go about implementing that though, or if it's even a
good approach. After all, you could potentially have multiple bash
builds of the same version with different configure options, which could
potentially leave out some function the loadable builtin expects to use.

-- 
Geir Hauge
diff --git a/builtins.h b/builtins.h
index 01565935..b4c71274 100644
--- a/builtins.h
+++ b/builtins.h
@@ -57,6 +57,7 @@ struct builtin {
   char * const *long_doc;  /* NULL terminated array of strings. */
   const char *short_doc;   /* Short version of documentation. */
   char *handle;/* for future use */
+  char *path;  /* path of loadable builtin. */
 };
 
 /* Found in builtins.c, created by builtins/mkbuiltins. */
diff --git a/builtins/enable.def b/builtins/enable.def
index 27d341a6..ae861b96 100644
--- a/builtins/enable.def
+++ b/builtins/enable.def
@@ -340,6 +340,7 @@ dyn_load_builtin (list, flags, filename)
 #endif
 
   handle = 0;
+  load_path = 0;
   if (absolute_program (filename) == 0)
 {
   loadables_path = get_string_value ("BASH_LOADABLES_PATH");
@@ -353,7 +354,6 @@ dyn_load_builtin (list, flags, filename)
 #else
  handle = dlopen (load_path, RTLD_LAZY);
 #endif /* !_AIX */
- free (load_path);
}
}
 }
@@ -377,6 +377,8 @@ dyn_load_builtin (list, flags, filename)
  if (name != filename)
free (name);
}
+  if (load_path)
+free (load_path);
   return (EX_NOTFOUND);
 }
 
@@ -434,6 +436,7 @@ dyn_load_builtin (list, flags, filename)
   if (flags & SPECIAL)
b->flags |= SPECIAL_BUILTIN;
   b->handle = handle;
+  b->path = load_path ? savestring(load_path) : NULL;
 
   if (old_builtin)
{
@@ -444,6 +447,9 @@ dyn_load_builtin (list, flags, filename)
  new_builtins[new++] = b;
 }
 
+  if (load_path)
+free (load_path);
+
   if (replaced == 0 && new == 0)
 {
   free (new_builtins);
diff --git a/builtins/type.def b/builtins/type.def
index a8e47c0a..fc7099db 100644
--- a/builtins/type.def
+++ b/builtins/type.def
@@ -52,6 +52,7 @@ $END
 
 #include 
 
+#include "../builtins.h"
 #include "../bashtypes.h"
 #include "posixstat.h"
 
@@ -289,6 +290,7 @@ describe_command (command, dflags)
 }
 
   /* Command is a builtin? */
+  struct builtin *b = builtin_address_internal(command, 0);
   if (((dflags & CDESC_FORCE_PATH) == 0) && find_shell_builtin (command))
 {
   if (dflags & CDESC_TYPE)
@@ -297,6 +299,8 @@ describe_command (command, dflags)
{
  if (posixly_correct && find_special_builtin (command) != 0)
printf (_("%s is a special shell builtin\n"), command);
+ else if (b->path)
+   printf (_("%s is a shell builtin (%s)\n"), command, b->path);
  else
printf (_("%s is a shell builtin\n"), command);
}

Re: wait inside subshell waits for sibling

[Emanuele Torre]

I don't think the process running `cat' is a sibling of the subshell.

bash performs an optimisation that runs redirections applied to simple
commands that run external programs, or subshell compound commands after
the fork(). (to avoid having to restore file descriptors after running
the command.)

This causes other weird pitfalls, e.g. the following script loops
infinitely, and always runs `sleep 1 > /tmp/file1':

i=0
while [ "$i" -lt 10 ]
do sleep 1 > "/tmp/file$(( ++i ))"
done

On 23/10/2022, Oğuz İsmail Uysal  wrote:
> To reproduce:
>
> $ echo $BASH_VERSION
> 5.2.2(3)-release
> $ ( : & wait ) > >(cat)
> *hangs*
>
> It should return immediately, but hangs instead.

This behaviour has existed for a while in bash, it was not introduced in
5.2.

It causes many pitfalls that are hard to spot; see [1] (which I just
updated to mention a problematic use case like yours), and [2].

[1]: 
[2]: https://www.vidarholen.net/contents/blog/?p=865

I have never encountered a problem like the one you described before,
but that is probably one of the worse pitfalls caused by this
optiomisation. :^)

Effectively, because of the optimisation, your command is run like:

(exec > >(cat); : & wait)

So the subshell process that runs `cat' is a child of the subshell that
runs `: & wait', so wait will also wait for it.

To inhibit this optimisation, you can wrap your subshell compound
command (or simple command) in a group command, and apply the
redirections to it instead of the subshell command:

{ (: & wait) ;} > >(cat)

Or, in your specific case, use "$!" to only wait for the most recent
background job.

(: & wait -- "$!") > >(cat)

Cheers.
 emanuele6

Re: wait inside subshell waits for sibling

[Oğuz]

24 Ekim 2022 Pazartesi tarihinde Emanuele Torre 
yazdı:
>
> To inhibit this optimisation, you can wrap your subshell compound
> command (or simple command) in a group command, and apply the
> redirections to it instead of the subshell command:
>
> { (: & wait) ;} > >(cat)
>
> Or, in your specific case, use "$!" to only wait for the most recent
> background job.
>
> (: & wait -- "$!") > >(cat)
>

Thanks, I'm not looking for a workaround.


-- 
Oğuz

Re: wait inside subshell waits for sibling

[Robert Elz]

Date:Mon, 24 Oct 2022 04:25:44 +0200
From:Emanuele Torre 
Message-ID:  


  | bash performs an optimisation that runs redirections applied to simple
  | commands that run external programs, or subshell compound commands after
  | the fork(). (to avoid having to restore file descriptors after running
  | the command.)

It need not make a difference.   The wait command is not intended to
be just the wait(2) (or wait(3) whichever it is on the system in
question) system call (or wrapper around another system call).

There's no reason that wait (the command) cannot check for candidate
jobs which are still running, and wait for any of those, and then
return as soon as there are none left.   That the shell happens to
have forked other processes for its own reasons should make no
difference at all.

  | This causes other weird pitfalls, e.g. the following script loops
  | infinitely, and always runs `sleep 1 > /tmp/file1':
  |
  | i=0
  | while [ "$i" -lt 10 ]
  | do sleep 1 > "/tmp/file$(( ++i ))"
  | done

That is an entirely different issue, and is working as is supposed
to work.   That it isn't what some people might expect is irrelevant.

kre