#375 Missing error handling of __wgetmainargs()

[Pali]

The msvcrt.dll function __wgetmainargs() since Windows XP version does not terminate process on error anymore (like it was before Windows XP) but instead returns non-zero value. Caller is then required to take action (e.g. terminate process or recover). Function __wgetmainargs() fails if memory allocation error occurs.

Note that the __wgetmainargs() function in versioned msvcrt library since msvcr70.dll (Visual C++ 2002) always returns int value (even if is used on pre-XP system). Only msvcrt.dll library has this unstable ABI, which in older versions has void return type (and terminates process) and in new versions has int return type.

mpg123 is calling msvcrt.dll __wgetmainargs() function from win32_cmdline_utf8(), which is defined in file https://mpg123.org/trunk/src/win32_support.c and is missing error handling.

mingw-w64 runtime in version from git contains ABI stabilization code wrapper which ensures that __wgetmainargs() always has int return value and zero value on success. Source code is here:
https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-crt/misc/msvcrt__wgetmainargs.c

If you are building mpg123 binaries only with Visual Studio 2002 and new (this use only version msvcrt library), or with mingw-w64 from recent git version (with ABI stabilization code) then you can use simple error handling by just checking for return value of __wgetmainargs(). For example by replacing line:

__wgetmainargs(argc, &argv_wide,&env,1, &startup);

by:

if (__wgetmainargs(argc, &argv_wide,&env,1, &startup) != 0) {
    error("Cannot allocate memory for command line.");
    return -1;
}

If you still want to support Visual C++ 6.0 or older mingw-w64, then you can use similar trick for error checking like in the mingw-w64 code. Note that Visual C++ 6.0 create binaries which uses system msvcrt.dll. For example replacing by:

*argc = -1;
argv_wide = NULL;
*env = NULL;
__wgetmainargs(argc, &argv_wide,&env,1, &startup);
if (*argc == -1 || argv_wide == NULL || env == NULL) {
    error("Cannot allocate memory for command line.");
    return -1;
}

This is not needed for AMD64 because first AMD64 version of msvcrt.dll is Windows Server 2003, which always returns int value (which has to be checked).

Note that I found this missing error handling in mpg123 code after I started investigating another reported bug that mpg123 was not possible to compile with the git version of mingw-w64 runtime:
https://github.com/mingw-w64/mingw-w64/issues/73 (this bug was in mingw-w64 codebase where is already fixed).

[manx]

patch attached

[Pali]

IMHO it would be better to use documented behavior by checking of __wgetmainargs return value when possible -- which is for non-i386 platforms, which could be done by:
#if !defined(__i386__) && !defined(_M_IX86)

__wgetmainargs is documented here:
https://learn.microsoft.com/en-us/cpp/c-runtime-library/getmainargs-wgetmainargs

[Thomas Orgis]

Seems like a pretty mess. Maybe https://manx.datengang.de/temp/wmain-v5.patch is a better route.

But in practice, we are talking of a very rare error case where we run out of memory processing command line arguments. How likely is it that the program will not crash shortly after that, anyway?

But of course early crashing/error exit is preferrable. I look at the further UTF-8 conversion calls and think that we need to handle failure there better when worrying about wgetmainargs.

[Pali]

I'm just giving suggestions here. It could be rather rare error, but sill possible.

I agree it is a mess and hard to write it in a way that supports all compilers, all build options, all Windows systems, all runtimes, etc...

To reduce this mess, I introduced that wrapper into mingw-w64 runtime, which ensures that for applications linking with new mingw-w64 runtime, the__wgetmainargsfunction always returns int. But new mingw-w64 version with that change was not released yet. So it does not help for older mingw-w64 versions, and neither if you are compiling with MSVC compiler.

If you want to reduce mess you can do it for example by dropping support for ANSI main builds and requiring UNICODE wmain builds. Then you do not need to care about wgetmainargs at all.

Note that there is -D_UNICODE and -DUNICODE. Underscore is for crt and non-underscore is for winapi. So for wmain you can just enforce -D_UNICODE, plus for gcc special switch -municode. Note that if you care about Win9x (possible reason why ANSI main is used), this wmain would work too as crt unicode is compatible also on old non-unicode systems (only winapi unicode is not available).

Anyway, if you have an idea how mingw-w64 runtime could help here, please let me know, so I can try to introduce some other fixups / wrappers in mingw-w64 code.

[Pali]

Just I'm curious, how are file names from argv[] handled after they are converted to UTF-8? Because windows needs either file names in UTF-16 (when using UNICODE api) or in ANSI encoding (when not using UNICODE api).

[manx]

They are converted back to wchar_t and passed to e.g. _wfopen. See for example INT123_compat_fopen in src/compat/compat.c.

Even though the perfect solution would be to always keep filenames in wchar_t on Windows, this is way too much hassle for an originally POSIX-focused project like mpg123. mpg123 tries to keep everything in char in UTF8 internally. Windows support and conversion handling is not perfect and has known bugs.

I am generally pushing for moving towards better Windows support (MinGW and MSVC), but my primary focus is on libmpg123 (which is arguably also the most important part), and less so on the program and other related libraries.

[Pali]

Thanks for info! It is really good to know for me what applications needs to do.

[Thomas Orgis]

Yeah, I decided on UTF-8 internal representation, also in the library APIs, as one universal choice. Of course this comes from POSIX environments, where the upgrade path from 8 bit encodings via UTF-8 is the great cheap trick.

Though, jumping to 16 bit wchar_t might also be considered a cheap trick, but a trick that failed once we had to move beyond UCS-2 into UTF-16, where you still have to deal with all the complexity of codepoint-vs-encoding-units …

I do have to wonder how much the mpg123 application is really used in Windows environments that are not WSL or Cygwin. It's causing some pain to maintain. Initially, I only ported libmpg123 to Windows … and for MSVC, we still do not attempt to build the applications themselves. And I don't know who misses that.

So it seems moving to wmain for native Windows stuff (in practice, only the mingw builds) is the way to go. I wonder if we could condense the changes a bit to avoid duplicating the entry point dance in each program source. Maybe even just a header bit/macro that expands to just

int main(int argc, char **argv)

for POSIX and the equivalent to get from wmain to UTF-8 argv for Windows, with another piece for cleanup at the end. At least some less duplication. I'm less afraid of macros than of repetitive code.

[Thomas Orgis]

Just an update: We are intending to move away from wgetmainargs, and rather switch to setting UNICODE mode and using wmain for that case. It'll take some weeks because of holiday season.

[Thomas Orgis]

Another update: I'm aiming at the small-scale return value handling now. The restructuring with wmain poses more questions and we lost our Windows maintainer over the somewhat heated discussions.

[Thomas Orgis]

@Pali: During testing the changes, can you give some hint on that error cross-compiling with mingw-w64 on Linux?

/bin/bash ./libtool  --tag=CC   --mode=link i686-w64-mingw32-gcc  -O2 -fomit-frame-pointer -funroll-all-loops -finline-functions -ffast-math  -Wall -Werror -std=c99 -pedantic -g -O2   -o src/mpg123.exe src/audio.o src/common.o src/sysutil.o src/control_generic.o src/equalizer.o src/getlopt.o src/httpget.o src/resolver.o src/genre.o src/mpg123.o src/metaprint.o src/local.o src/playlist.o src/streamdump.o src/term.o  src/term_win32.o    src/net123_winhttp.o src/net123_wininet.o src/win32_support.o src/win32_net.o src/compat/libcompat.la src/libmpg123/libmpg123.la src/libout123/libout123.la src/libsyn123/libsyn123.la -lm  -lwinhttp -lwininet -lws2_32 
libtool: link: i686-w64-mingw32-gcc -O2 -fomit-frame-pointer -funroll-all-loops -finline-functions -ffast-math -Wall -Werror -std=c99 -pedantic -g -O2 -o src/.libs/mpg123.exe src/audio.o src/common.o src/sysutil.o src/control_generic.o src/equalizer.o src/getlopt.o src/httpget.o src/resolver.o src/genre.o src/mpg123.o src/metaprint.o src/local.o src/playlist.o src/streamdump.o src/term.o src/term_win32.o src/net123_winhttp.o src/net123_wininet.o src/win32_support.o src/win32_net.o  src/compat/.libs/libcompat.a src/libmpg123/.libs/libmpg123.dll.a src/libout123/.libs/libout123.dll.a -lshlwapi src/libsyn123/.libs/libsyn123.dll.a -lwinhttp -lwininet -lws2_32 -L/usr/local/lib
src/.libs/lt-mpg123.c:41:5: error: '_putenv' redeclared without dllimport attribute: previous dllimport ignored [-Werror=attributes]
   41 | int _putenv (const char *);
      |     ^~~~~~~
cc1: all warnings being treated as errors

[Pali]

It looks like that you have own declaration of _putenv function in one of your source files, which is incompatible with the declaration already included from system header file.

The best practice is to just include appropriate system header file and do not write own declaration of system functions.

In this particular case the function declaration in your source file and in system header file differs on the __attribute__((dllimport)) keyword.

[Thomas Orgis]

Yeah, except I don't find a declaration of putenv, let alone use of it in our sources. This is in files that start with

/* src/.libs/lt-mpg123-strip.c - temporary wrapper executable for .libs/mpg123-strip.exe
   Generated by libtool (GNU libtool) 2.4.6 Debian-2.4.6-15build2

   The src/mpg123-strip program cannot be directly executed until all the libtool
   libraries that it depends on are installed.

   This wrapper executable should never be moved out of the build directory.
   If it is, it will not operate correctly.
*/

So this is a libtool bug and should be reported there?

(PS: right now I am confused by trying to reproduce inconsistent printout of Unicode characters to the terminal, where simple printf of UTF-8 to a windows console configured with chcp 65001 gives garbled output with our binaries since mpg123 1.26.0, but worked with 1.25 series … stumbling over missing libwinpthread1.dll in builds from the msys2 ucrt shell (what's the difference with the msys2 shell? maybe I took a wrong turn there) and observing that those binaries seem to printout utf-8 just fine, but the cross-compiled binaries we produced on Linux seem not to … unsure. Encodings for printouts are a mystery and this is not helped by wine and the wineconsole under Linux not behaving like the windows cmd console int hat respect.)

[Pali]

So this is a libtool bug and should be reported there?

I'm not very familiar with libtool and how it works for windows. But from your description it looks like that the file was autogenerated by libtool. And in this case it can be a bug in libtool.

About encoding, it is hard to say. I do not know.

[Thomas Orgis]

I think we handled this in 1.33.0.