Fix tools tty detection (fix avrdude running in safe mode)

[cmaglie]

Some tools detects if the program is running from terminal by looking at the stdin/out bindings.
Previously stdin wasn't bound to any custom stream, this fact lead avrdude to think it was run from terminal (instead of a script) and start it in "safe-mode". This turn out to be a problem in some cases, see #844

From the avrdude source code:

  if (isatty(STDIN_FILENO) == 0)
      safemode  = 0;       /* Turn off safemode if this isn't a terminal */

So we need to ensure that isatty(STDIN_FILENO) returns 0 when we run tools.
I've tried to run the following test program:

#include <stdio.h>
#include <unistd.h>
#include <io.h>

int main(void) {
    printf("%d\n", isatty(STDIN_FILENO));
    return 0;
}

compiled with:

x86_64-pc-cygwin-gcc test.c -o test-cygwin

and

i686-w64-mingw32-gcc test.c -o test-mingw32

using the following launcher in go:

func main() {
	cmd, err := executils.Command([]string{"test-cygwin.exe"})
	if err != nil {
		log.Fatalf("cannot execute upload tool: %s", err)
	}

	var o, e bytes.Buffer
	outStream := bufio.NewWriter(&o)
	errStream := bufio.NewWriter(&e)
	inStream := bufio.NewReader(&o)
	cmd.Stdout = outStream
	cmd.Stderr = errStream
	cmd.Stdin = inStream      // <---
	if err := cmd.Start(); err != nil {
		log.Fatalf("cannot execute upload tool: %s", err)
	}

	err = cmd.Wait()
	fmt.Println(o.String())
	if err != nil {
		log.Fatalf("uploading error: %s", err)
	}
}

I tried with and without the cmd.Stdin = inStream:

• the cygwin build is unaffected it always returns 0
• the mingw32 build instead returns 64 without the stdin redirect and 0 with the stdin redirect.

This makes me think that avrdude has been built with mingw32 and this PR should fix the issue, but a direct test is required.

Fix: #844

[matthijskooijman]

If running with safemode is the problem here, wouldn't a cleaner fix be to run avrdude with -s to disable safemode explicitly? That would also need to happen in third-party cores, so that's a bit more tricky. OTOH, thinking on this again, since we're not sending any input to avrdude, closing its stdin or redirecting it to null would be fine.

Looking at the code, this adds custom readers and writers to create null streams, which seems rather inefficient to me (since it involves creating an extra pipe pair underwater, I think). A simpler way would be to just set .e.g Stdin to nil, which redirects to /dev/null (or the equivalent), see docs. Did you consider that option?

[cmaglie]

that would also need to happen in third-party cores

yes, that's the problem, adding -s won't fix the issue in general. This is worsened by the fact that this happens only with the arduino-cli and not with the Arduino IDE, so 3rd party maintainers are less encouraged to fix it.

Looking at the code, this adds custom readers and writers to create null streams, which seems rather inefficient to me (since it involves creating an extra pipe pair underwater, I think). A simpler way would be to just set .e.g Stdin to nil, which redirects to /dev/null (or the equivalent), see docs. Did you consider that option?

I tried that, but they are nil by default, setting it explicitly doesn't seem to affect the call to isatty.
I don't know which checks performs isatty under mingw32 but, if we want it to return 0, it seems that we need to create a completely different stream. BTW a pipe has a very little cost in term of efficiency, considering that we already use 2 to get the stdout and stderr.

[matthijskooijman]

I tried that, but they are nil by default, setting it explicitly doesn't seem to affect the call to isatty.

Hm, good point.

In the original report @per1234 mentions this problem occurs on Windows and Linux/Ubuntu, but you're only mentioning Windows here. I also tried reproducing it (with your testcode, not full arduino-cli) on Ubuntu, but there it seems to work as expected (istty returning false for /dev/null). Did anything change since that report already maybe?

Looking at the golang exec package, it seems that Stdin = nil means it opens os.DevNull:

https://github.com/golang/go/blob/d0d6593d1d4e81acd073244f42b6893fa65c99d8/src/os/exec/exec.go#L245-L252

which on windows is just a special file called NUL:

https://github.com/golang/go/blob/e64675a79fef5924f268425de021372df874010e/src/os/file_windows.go#L97

So that sounds like it should work. Maybe isatty does something funny on mingw32 with NUL somehow?

This post suggests that the "64 return value for NUL" is actually the case with _isatty, which is a Windows-specific function (docs) that determines whether an FD is a character device (rather than an actually tty, like Linux isatty), and I suppose NUL might be a character device. I also suppose that mingw might just substitute isatty() with _isatty() (also suggested here: https://sourceforge.net/p/mingw-w64/mailman/message/35498600/). It seems that this still happens with mingw git version (see file comment here), not sure about different mingw versions, though.

It seems that the ideal fix for this would maybe be in mingw, which would then be used to recompile a fixed avrdude. However, that's quite complicated, so maybe your solution is indeed the easiest way forward. I do wonder if this redirection needs to happen for stdout/stderr as well (seems to be overridden in practice), maybe just keep the hack as small as possible and only apply it for stdin?

One alternative could be to simply close stdin rather than redirect it to /dev/null, which I think would also make isatty return false. However, it seems that the golang exec module does not support this in any way (it would involve stdin() returning nil to pass a nil file descriptor to os.startProcess around this bit of code, but I don't think we can override this in such a way.

[per1234]

In the original report @per1234 mentions this problem occurs on Windows and Linux/Ubuntu

Sorry, I think my report was unclear. When I said this:

If you run the identical upload command from the command line on Linux or Windows, you get the same failure shown above.

I meant running the avrdude command directly, not using arduino-cli.

[per1234]

I verified that I can still reproduce #844 with Arduino CLI @master, but that #844 no longer occurs when using Arduino CLI @ 82a957f.

Thanks @cmaglie!