Add --color command line argument to control colored console output

[Calinou]

• Follow-up to Add colors to the command-line help #36252.

This also adds support for the NO_COLOR environment variable, and harmonizes checks for colored console output by using a unique Engine method.

• auto (default): Only color if output is a TTY (interactive session). Color is disabled if the NO_COLOR environment variable is set to a non-empty string, as per https://no-color.org.
• always: Always color, even if output is a file or is non-interactive (useful for continuous integration, which is non-interactive). This also bypasses NO_COLOR as per its specification.
• never: Never color command line output.

Preview

[Calinou]

I've tried to fix the build on Windows, but am not sure how to fix the heap-use-after-free in the Linux build:

==7218==ERROR: AddressSanitizer: heap-use-after-free on address 0x612000009650 at pc 0x5577491682f7 bp 0x7fff032db4d0 sp 0x7fff032db4c0
READ of size 8 at 0x612000009650 thread T0
    #0 0x5577491682f6 in UnixTerminalLogger::log_error(char const*, char const*, int, char const*, char const*, bool, Logger::ErrorType) (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x45dfb2f6)
    #1 0x55775ada11f3 in CompositeLogger::log_error(char const*, char const*, int, char const*, char const*, bool, Logger::ErrorType) (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x57a341f3)
    #2 0x55775a71fe05 in OS::print_error(char const*, char const*, int, char const*, char const*, bool, Logger::ErrorType) (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x573b2e05)
    #3 0x55775c1f4cf9 in _err_print_error(char const*, char const*, int, char const*, char const*, bool, ErrorHandlerType) (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x58e87cf9)
    #4 0x55775c1f49ab in _err_print_error(char const*, char const*, int, char const*, bool, ErrorHandlerType) (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x58e879ab)
    #5 0x55775bf1c1ca in ObjectDB::cleanup() (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x58baf1ca)
    #6 0x55775a5520bd in unregister_core_types() (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x571e50bd)
    #7 0x55773fd09e89 in Main::test_cleanup() (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x3c99ce89)
    #8 0x55773fd0a29d in Main::test_entrypoint(int, char**, bool&) (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x3c99d29d)
    #9 0x55773fa8be31 in main (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x3c71ee31)
    #10 0x7f861c5b0082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082)
    #11 0x55773fa8bbad in _start (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x3c71ebad)

0x612000009650 is located 16 bytes inside of 264-byte region [0x612000009640,0x612000009748)
freed by thread T0 here:
    #0 0x7f861d37040f in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:122
    #1 0x55775a71a7c8 in Memory::free_static(void*, bool) (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x573ad7c8)
    #2 0x55773fd8c5c4 in void memdelete<Engine>(Engine*) (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x3ca1f5c4)
    #3 0x55773fd09e70 in Main::test_cleanup() (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x3c99ce70)
    #4 0x55773fd0a29d in Main::test_entrypoint(int, char**, bool&) (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x3c99d29d)
    #5 0x55773fa8be31 in main (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x3c71ee31)
    #6 0x7f861c5b0082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082)

previously allocated by thread T0 here:
    #0 0x7f861d370808 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144
    #1 0x55775a71976d in Memory::alloc_static(unsigned long, bool) (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x573ac76d)
    #2 0x55775a71967e in operator new(unsigned long, char const*) (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x573ac67e)
    #3 0x55773fd079a1 in Main::test_setup() (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x3c99a9a1)
    #4 0x55773fd0a284 in Main::test_entrypoint(int, char**, bool&) (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x3c99d284)
    #5 0x55773fa8be31 in main (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x3c71ee31)
    #6 0x7f861c5b0082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082)

The problematic line is https://github.com/Calinou/godot/blob/4dd89b440234c8e1b4d7e6da748d20d859049f0f/drivers/unix/os_unix.cpp#L794. I'm just not sure how to avoid having to query the Engine singleton here, while keeping the solution as local as possible.

[dbnicholson]

The problematic line is https://github.com/Calinou/godot/blob/4dd89b440234c8e1b4d7e6da748d20d859049f0f/drivers/unix/os_unix.cpp#L794. I'm just not sure how to avoid having to query the Engine singleton here, while keeping the solution as local as possible.

Am I wrong or is this is simple as not blindly dereferencing the Engine singleton since it may have already been freed?

diff --git a/drivers/unix/os_unix.cpp b/drivers/unix/os_unix.cpp
index 8ca6f8c668..1e2ea5534d 100644
--- a/drivers/unix/os_unix.cpp
+++ b/drivers/unix/os_unix.cpp
@@ -791,7 +791,8 @@ void UnixTerminalLogger::log_error(const char *p_function, const char *p_file, i
                err_details = p_code;
        }
 
-       const bool should_color = Engine::get_singleton()->is_coloring_standard_output();
+       Engine *engine = Engine::get_singleton();
+       const bool should_color = engine ? engine->is_coloring_standard_output() : false;
        const char *gray = should_color ? "\E[0;90m" : "";
        const char *red = should_color ? "\E[0;91m" : "";
        const char *red_bold = should_color ? "\E[1;31m" : "";

[dbnicholson]

Not that I want to throw out this PR, but the API feels like it should be a part of OS since that layer is below Engine and the OS layer is where knowledge of whether coloring can be done or not (e.g., isatty()) should take place.

So, if I was to propose an alternate API, move color_standard_output and the getter/setter to OS. In the OS::OS constructor, add the NO_COLOR environment variable check to set it to false if needed. In OS_Unix::OS_Unix, further enhance the default by checking isatty(fileno(stdout)). Since the OS singleton outlives the Engine singleton, the use after free issue is also avoided.

[Calinou]

Rebased and tested again, it works as expected. I've also added CLI argument validation for --color.

Am I wrong or is this is simple as not blindly dereferencing the Engine singleton since it may have already been freed?

That works, thanks 🙂