lcn2 / calc

C-style arbitrary precision calculator
http://www.isthe.com/chongo/tech/comp/calc/index.html
Other
346 stars 52 forks source link

Bug: prompt() doesn't work in calc shell script mode or from command line args #133

Closed gromit1811 closed 8 months ago

gromit1811 commented 10 months ago

Don't know whether this is actually a bug or just a misunderstanding: If I try to use prompt() in calc shell script mode (i.e. from a script file read by option "-f") or when calling prompt() from a command line argument, it just prints its prompt but doesn't wait for input and always returns null. If I understand correctly, this is because calc closes stdin in both cases so prompt() obviously can't read anyhting.

Now: Is closing stdin in these cases intentional? At least to me (and to the person reporting this bug in Debian long ago) it was unexpected. How is prompt() supposed to be used then? Interactively from calc's command line it doesn't make too much sense either IMO.

I noticed that I can prevent stdin from getting closed using "-p". Is that how prompt() is supposed to be used in these cases?

If anybody can shed some light on the intentions here, I can probably submit a fix for either code or man page, depending on where the problem is.

To Reproduce

$ cat prompt_test.cal 
#!/usr/bin/calc -q -f

n = prompt("Test> ");
if (isnull(n)) {
  print("got null");
} else {
  print("got " + n);
}

$ ./prompt_test.cal 
Test> got null
$ 

Expected behavior

To get the expected behavior, change the first line in the script to:

#!/usr/bin/calc -q -p -f

Then prompt() actually waits for input and returns it:

$ ./prompt_test.cal 
Test> INPUT
got INPUT
$ 

Execution environment (please complete the following information):

lcn2 commented 10 months ago

Interesting report @gromit1811 .. we will look into this issue as well.

lcn2 commented 10 months ago

On MacOS 14.1, @gromit1811:

./prompt_test.cal
Test> got null
"./prompt_test.cal", line 5: Extraneous right brace

Error in commands

On MacOS 14.1, when we use the 1st line of "#!/usr/bin/calc -q -p -f we get:

$ ./prompt_test.cal
Test> INPUT
got INPUT

On MacOS 14.1, interactive mode works:

$ calc
C-style arbitrary precision calculator (version 2.15.0.1)
Calc is open software. For license details type:  help copyright
[Type "exit" to exit, or "help" for help.]

; n = prompt("Test> ");
Test> INPUT
; n
    "INPUT"

On Linux RHEL9.3:

$ ./prompt_test.cal
Test> got null

When on Linux RHEL9.3, when we use the 1st line of "#!/usr/bin/calc -q -p -f we get:

/prompt_test.cal
Test> INPUT
got INPUT

On MacOS RHEL9.3, interactive mode works:

$ calc
C-style arbitrary precision calculator (version 2.15.0.1)
Calc is open software. For license details type:  help copyright
[Type "exit" to exit, or "help" for help.]

; n = prompt("Test> ");
Test> INPUT
; n
    "INPUT"

Hummm ...

lcn2 commented 10 months ago

Interesting, @gromit1811, consider this script (./prompt_test2.cal):

#!/usr/bin/calc -q -f

n = prompt("Test> ");
This is what prompt will grab
if (isnull(n)) {
  print("got null");
} else {
  print("got " + n);
}

This will produce:

$ ./prompt_test2.cal
Test> got This is what prompt will grab

That is, when in "SHELL SCRIPT MODE" (see the calc man page), with without -p, the prompt() builtin will grab the next line of the file. Why? Well in "SHELL SCRIPT MODE" calc is reading and executing commands read fin from standard input (i.e., stdin). However when the prompt() builtin is executed, that builtin function will also grab the next line from prompt() builtin as well.

When we use ./prompt_test.cal:

#!/usr/bin/calc -q -f

n = prompt("Test> ");
if (isnull(n)) {
  print("got null");
} else {
  print("got " + n);
}

and that script produces:

$ Test> got null
"./prompt_test.cal", line 5: Extraneous right brace

Error in commands

The prompt builtin grabs the next line (i.e., "if (isnull(n)) {"). Then the command parser processes the next line (i.e., " print("got null");) and so "got null" is printed. Next the command parser see the next lines, which because the command parser did not see the initial if, the command line parser reports the syntax error "Extraneous right brace".

Consider this ./prompt_test3.cal file:

#!/usr/bin/calc -q -f

n = prompt("Test> ");

if (isnull(n)) {
  print("got null");
} else {
  print("got " + n);
}

The prompt grabs the empty line from standard input, which is an empty line, and the command line parser parsers the rest of the file, which in this case is NOT a syntax error:

$ ./prompt_test3.cal
Test> got

So the issue is this:

When the command line intercepter in "SHELL SCRIPT MODE" is reading commands from standard input, the prompt builtin will read the next line of the script from standard input.

lcn2 commented 9 months ago

Hello @gromit1811,

This is looking more and more like an "feature" that needs to be documented.

We are looking for a potential work-a-round, but it appears we are dealing with a conflict reading stdin between the prompt() and the script execution.

Stay tunes ...

gromit1811 commented 9 months ago

I think now I'm completely confused. Why does prompt() conflict with script reading at all? And why does it do so only on MacOS?

This is my current understanding: When using an executable script starting with #!, the OS executes the specified script interpreter and passes it the file name of the script as an argument. Since we use -f as the last option in the first line, calc itself opens that file and reads commands from it.

Now one of those commands is prompt(). This calls f_prompt() and that wraps its call to nextline() (which ultimately reads from stdin) in an openterminal()/closeinput() pair which manages a stack of input sources. So this way, calc prevents internal confusion between different input sources. On Linux, this seems to work as intended, but on MacOS it doesn't. Why? Is the script actually passed on stdin on MacOS? I can't believe that's the case, but then, how else would prompt() be able to read a line of the script?

Can you clarify my confusion?

lcn2 commented 9 months ago

I think now I'm completely confused. Why does prompt() conflict with script reading at all? And why does it do so only on MacOS?

This is my current understanding: When using an executable script starting with #!, the OS executes the specified script interpreter and passes it the file name of the script as an argument. Since we use -f as the last option in the first line, calc itself opens that file and reads commands from it.

Now one of those commands is prompt(). This calls f_prompt() and that wraps its call to nextline() (which ultimately reads from stdin) in an openterminal()/closeinput() pair which manages a stack of input sources. So this way, calc prevents internal confusion between different input sources. On Linux, this seems to work as intended, but on MacOS it doesn't. Why? Is the script actually passed on stdin on MacOS? I can't believe that's the case, but then, how else would prompt() be able to read a line of the script?

Can you clarify my confusion?

At the moment we cannot fully clarity @gromit1811, which is why we have not declared a solution.

UPDATE 0

A key phase we used was "appears we are dealing with" meaning it isn't actually this way, it just seems that way. It "appears" we should have been even more vague in our previous update. :-)

UPDATE 1

This is my current understanding: When using an executable script starting with #!, the OS executes the specified script interpreter and passes it the file name of the script as an argument. Since we use -f as the last option in the first line, calc itself opens that file and reads commands from it.

Now one of those commands is prompt(). This calls f_prompt() and that wraps its call to nextline() (which ultimately reads from stdin) in an openterminal()/closeinput() pair which manages a stack of input sources. So this way, calc prevents internal confusion between different input sources. On Linux, this seems to work as intended,

Correct.

but on MacOS it doesn't. Why? Is the script actually passed on stdin on MacOS? I can't believe that's the case,

On macOS, the following executable script (call it myecho):

#!/bin/echo
This line does nothing

End of stuff

When run as

./myecho

Outputs:

./myecho

And when run as:

./myecho a b c

Will output:

./myecho a b c

Then this executable script when run on macOS:

#!/bin/cat
Stuff that the cat ignores.

Meow!

When run as:

./mycat

Will output the contents of mycat.

And when run under macOS as:

./mycat myecho

Will output the contents of mycat followed by the contents of myecho.

And when run under macOS as:

./mycat /dev/stdin

This will output the contents of mycat and start reading from standard in, echoing typed stuff until EOF.

but then, how else would prompt() be able to read a line of the script?

We are not sure. Any guesses @gromit1811?

gromit1811 commented 9 months ago

I've got a Mac VM now, so I could experiment with this as well. It's actually quite simple once you understand what's going on (but isn't it always like this?)...

With -f and without -p, calc closes stdin (why is still unclear to me - that's the fundamental question in this issue). It subsequently opens the script file for reading. Because it just closed stdin, file descriptor 0 became available, so the script file now gets opened with file descriptor 0. If we then try to read from stdin (the FILE * we've just closed and not reopend but maybe still with a reference to file descriptor 0 which it used before closing), results differ between MacOS and Linux: On Linux, you get a "Bad file descriptor", on MacOS, reading from the closed stdin actually reads from the file now open with file descriptor 0. So when prompt() tries to read a line from stdin, on MacOS it will actually consume a line from the script file as you noticed.

Test case:

#include <stdio.h>

int main() {
  int res;
  FILE *f;

  res = fclose(stdin);
  if (res == EOF) {
    perror("fclose");
    return 1;
  }

  f = fopen("close_stdin.c", "r");
  if (!f) {
    perror("fopen");
    return 1;
  }

  printf("fileno: %d\n", fileno(f));

  res = fgetc(f);
  if (res == EOF) {
    perror("fgetc(f)");
    return 1;
  }
  printf("fgetc(f) after fopen: %c\n", res);

  res = fgetc(stdin);
  if (res == EOF) {
    perror("fgetc(stdin)");
    return 1;
  }
  printf("fgetc(stdin) after fopen: %c\n", res);

  return 0;
}

Output on Linux:

$ ./close_stdin 
fileno: 0
fgetc(f) after fopen: #
fgetc(stdin): Bad file descriptor

Output on MacOS:

% ./close_stdin 
fileno: 0
fgetc(f) after fopen: #
fgetc(stdin) after fopen: i

Both behaviors are OK, because:

$ man fclose
[...]
RETURN VALUE
       Upon successful completion, 0 is returned.  Otherwise, EOF is returned and er‐
       rno is set to indicate the error.  In either case, any further access (includ‐
       ing another call to fclose()) to the stream results in undefined behavior.

-> undefined behavior, anything might happen.

So: If we conclude that closing stdin actually serves a purpose, it's probably better to freopen("/dev/null", "r") instead. This way, fd 0 becomes inaccessible, but reading from stdin doesn't trigger undefined behavior and will simply yield EOF. Problem: What do we do on Windows without /dev/null?

Or we simply stop closing stdin. But I guess it was added for a reason, so to make that decision, we should probably try to understand why. Problem is that git blame is not really helpful in that case, because this change was made a long time ago and git only has one commit per release from that time.

gromit1811 commented 9 months ago

Looks like fclose(stdin) was added in 2013 in release 2.12.4.9. And CHANGES has an entry that could be relevant:

+    Fixed bug was uncovered in calc that caused script failures when calc
+    is called within a while loop in BASH if the while loop is fed from
+    stdin due to calc's redirection/inheritance of stdin and no option
+    to change this behavior.  Thanks gores to David C. Rankin
+    <drankinatty at gmail dot com> for the bug fix and to David Haller
+    <dnh at opensuse dot org> for helping debug this problem.

So it seems to serve a purpose, but I still don't fully understand which one. Do you maybe have more context information from that time?

lcn2 commented 9 months ago

Looks like fclose(stdin) was added in 2013 in release 2.12.4.9. And CHANGES has an entry that could be relevant:

+    Fixed bug was uncovered in calc that caused script failures when calc
+    is called within a while loop in BASH if the while loop is fed from
+    stdin due to calc's redirection/inheritance of stdin and no option
+    to change this behavior.  Thanks gores to David C. Rankin
+    <drankinatty at gmail dot com> for the bug fix and to David Haller
+    <dnh at opensuse dot org> for helping debug this problem.

So it seems to serve a purpose, but I still don't fully understand which one. Do you maybe have more context information from that time?

Hmmm @gromit1811 ... here is the GitHub commit for 2.12.4.9:

The text you sited in CHANGES, according to the above commit URL has a header of:

The following are the changes from calc version 2.12.4.3 to 2.12.4.5:

If you look at diffs for calc.c we see:

calc c-diff 2 12 4 9

Regarding the background for this change:

Here is a message from David C. Rankin ( drankinatty@gmail.com ) on 2012 June 2:

bug.report.20120602.txt

And then on 2012 June 14:

bug.report.20120614.txt

And then on 2012 June 19:

bug.report.20120619.txt

Comment welcome.

lcn2 commented 9 months ago

And @gromit1811 regarding:

So: If we conclude that closing stdin actually serves a purpose, it's probably better to freopen("/dev/null", "r") instead. This way, fd 0 becomes inaccessible, but reading from stdin doesn't trigger undefined behavior and will simply yield EOF. Problem: What do we do on Windows without /dev/null?

Interesting idea. We applied this patch:

diff --git a/calc.c b/calc.c
index 7573f6d..af33ea1 100644
--- a/calc.c
+++ b/calc.c
@@ -543,8 +543,8 @@ main(int argc, char **argv)
                cmdbuf[cmdlen++] = '\n';
                cmdbuf[cmdlen] = '\0';
                if (p_flag != true) {
-                       if (fclose(stdin)) {
-                               perror("main(): fclose(stdin) failed:");
+                       if (freopen("/dev/null", "r", stdin) == NULL) {
+                               perror("in main: freopen(\"/dev/null\", \"r\", stdin) failed:");
                        }
                        stdin_closed = true;
                }

When we tried calc patch and then ran the following script (./prompt_test1.cal):

#!/usr/bin/calc -q -f

n = prompt("Test> ");
if (isnull(n)) {
  print("got null");
} else {
  print("got " + n);
}

We got:

Test> got null

Feel free to try the above patch yourself, we think you might find that the patch does not resolve the issue you raised.

Nevertheless, it seems like a good idea to apply this patch so that a later open will not grab the 1st file descriptor. So we plan to use this in the next version of calc.

Or we simply stop closing stdin. But I guess it was added for a reason, so to make that decision, we should probably try to understand why. Problem is that git blame is not really helpful in that case, because this change was made a long time ago and git only has one commit per release from that time.

The problem with this patch:

diff --git a/calc.c b/calc.c
index 7573f6d..f6898de 100644
--- a/calc.c
+++ b/calc.c
@@ -542,12 +542,6 @@ main(int argc, char **argv)
        if (havecommands) {
                cmdbuf[cmdlen++] = '\n';
                cmdbuf[cmdlen] = '\0';
-               if (p_flag != true) {
-                       if (fclose(stdin)) {
-                               perror("main(): fclose(stdin) failed:");
-                       }
-                       stdin_closed = true;
-               }
        }

        argc_value = argc - maxindex + 1;

is that while this "fixes" the problem you noted, is creates other problems.

For example, the following shell command:

$ calc show builtin

normally will print the list of buitin functions and exit. But with the above patch, it stalls waiting input on stdin.

One has to do this to get it not stall waiting for input on stdin:

$ calc show builtin < /dev/null

The make rule:

$ make testfuncsort

will stall for the same reason.

The above set of problems is what the original patch to close stdin (unless using -p) was designed to address. The make testfuncsort is just one example of problems that arise when using calc inside a shell.

And while there are workaround for the above (such as via "< /dev/null"), the impact of not closing stdin is probably non-trivial problems and adverse side effects, not to mention issues with using calc in shell scripts.

See the "help script" for more examples of documented use of calc in shell scripts that would be impacted by not closing stdin (unless -p was given).

So it seems that this prompt problem, and the need to close stdin (or better yet, freopen stdin as /dev/null in the patch above) has the hallmark of being a "it's not a bug it's a feature". If this is the case, then the problem with using the prompt builtin in a shell script / calc script without using calc -p should be documented in help/prompt, and help/unexpected as well as the calc man page.

Comments welcome.

lcn2 commented 9 months ago

Hello @gromit1811,

Here is how we propose to resolve this issue #133:

issue.133.ptch.txt

Comments, suggestions and corrections are welcome.

lcn2 commented 8 months ago

We are testing the above mentioned fix for this issue along with a fix for issue #135. Once we are finished testing we will release a new version of calc and close this issue.

Comments, suggestions and corrections are welcome.

lcn2 commented 8 months ago

Hello @gromit1811,

If you want to see the proposed fix to this issue, check out the master brach.

See commit 932d27053e12f64ab5041fc51df9422202c80f57 or later, although most of the work was done in the earlier commit 54dd89dcf7378e190602b603713ccf7b930ff28b.

gromit1811 commented 8 months ago

I've not actually tested it yet, but the diff looks good to me. The fact that stdin gets closed in shell script mode is now hopefully mentioned in a sufficient number of places. And the freopen instead of fclose should avoid surprises on MacOS and maybe other platforms when not using -p.

What I can't judge is what Windows will do when trying to open /dev/null. README.WINDOWS mentions lots of variants how to build calc on Windows. Cygwin should have /dev/null, but what about the other build options? Or is your strategy "wait and see whether somebody complains"? Fine for me, I don't have a suitable Windows I could use to test this on anyway.

lcn2 commented 8 months ago

I've not actually tested it yet, but the diff looks good to me. The fact that stdin gets closed in shell script mode is now hopefully mentioned in a sufficient number of places. And the freopen instead of fclose should avoid surprises on MacOS and maybe other platforms when not using -p.

What I can't judge is what Windows will do when trying to open /dev/null. README.WINDOWS mentions lots of variants how to build calc on Windows. Cygwin should have /dev/null, but what about the other build options? Or is your strategy "wait and see whether somebody complains"? Fine for me, I don't have a suitable Windows I could use to test this on anyway.

Thank you @gromit1811 for your reply AND for asking your question as the answer helped us understand why stdin is closed AND hopefully will help others understand the use of -p in scripts.

We will now close your issue with our thanks! 🙏