cmd: make string/error code more robust against errno leaking

[mvo5]

The i386 sid sbuild fails because apparently some sbuild code
is calling functions that are not implemented so errno is set
during the tests when it is not expected. This leads to test
failures because the die() code will append errno status if
errno is set. This commit fixes this and makes the nightly
test also run on i386.

[bboozzoo]

The build log is here: https://buildd.debian.org/status/fetch.php?pkg=snapd&arch=i386&ver=2.48.2-2&stamp=1610734191&raw=0

There were errors in unit tests:

# child process (/string-utils/sc_string_quote__NULL_buf [728]) stderr: "cannot quote string: string is NULL: Function not implemented\n"
**
ERROR:libsnap-confine-private/string-utils-test.c:471:test_sc_string_quote_NULL_str: stderr of child process (/string-utils/sc_string_quote__NULL_buf [728]) failed to match: cannot quote string: string is NULL

stderr was:
cannot quote string: string is NULL: Function not implemented

# ERROR:libsnap-confine-private/string-utils-test.c:471:test_sc_string_quote_NULL_str: stderr of child process (/string-utils/sc_string_quote__NULL_buf [728]) failed to match: cannot quote string: string is NULL

stderr was:
cannot quote string: string is NULL: Function not implemented

not ok 96 /string-utils/sc_string_quote__NULL_buf
ok 97 /string-utils/sc_string_append_char_pair__uninitialized_buf
ok 98 /string-utils/sc_string_quote
ok 99 /string-utils/sc_strdup
# Start of sc_must_snprintf tests
# child process (/string-utils/sc_must_snprintf/fail [730]) exit status: 1 (error)
# child process (/string-utils/sc_must_snprintf/fail [730]) stdout: ""
# child process (/string-utils/sc_must_snprintf/fail [730]) stderr: "cannot format string: 1234: Function not implemented\n"
**
ERROR:libsnap-confine-private/string-utils-test.c:95:test_sc_must_snprintf__fail: stderr of child process (/string-utils/sc_must_snprintf/fail [730]) failed to match: cannot format string: 1234

stderr was:
cannot format string: 1234: Function not implemented

# ERROR:libsnap-confine-private/string-utils-test.c:95:test_sc_must_snprintf__fail: stderr of child process (/string-utils/sc_must_snprintf/fail [730]) failed to match: cannot format string: 1234

stderr was:
cannot format string: 1234: Function not implemented

I've looked at the errors are both use g_test_subprocess(), however there are other tests using the same, which were successful. Also, the error in the tested code happens right away, before any heavy code runs. I'm counting only va_start() and sc_panicv(), vfprintf() on the call path. However, the errno is copied before vfprintf(), so it's either va_start() (which afaik does not set errno), or something that happens before the test, maybe some glib test integration left it.

Now, thinking about a proper fix, there's some other code that could die in sanity checks, that has not called any 3rd party library code, and for which errno could have been mangled before. There's also some other code which actually sets errno to indicate errors. Looks like we should review all those calls and fix errno handling there.

[bboozzoo]

maybe it should be errno = EINVAL in this branch and likely in other places doing sanity checks? Though this would break the test, as they expect this particular error string, without the following : Invalid argument🤔

[mvo5]

Thanks, yeah, this PR tries to fix the issue in a minimal way but we can of course do a followup that does more changes.

[mvo5]

Fwiw, I used this as a distro patch to unblock the debian build and with that we have a clean 2.48.3 build in unstable now: https://tracker.debian.org/pkg/snapd

[bboozzoo]

LGTM. Let's land it and start looking into cleanup the errno mess in our internal libraries.

[stolowski]

Shouldn't we save & restore errno before returning, or am I paranoid?

[bboozzoo]

It depends on whether the API promises to not modify errno. In this case I would say no, there's a separate call sc_error_init_from_errno which takes a copy of errno if you want to set in the error. There's a separate SC_ERRNO_DOMAIN domain for errors carrying errno, such that when initializing the error one is expected to pass the errno (copy thereof) in the arguments.

[mvo5]

I think we can. Note that we plan to change the API of die() to something like die() and die_with_errno() - once that is done this errno = 0; will be removed again.

[stolowski]

My main concern is obviously cases where sc_error_initv(..) is used to create and initialize sc_error, but after calling it we want to do something extra based on last errno value; and yes @bboozzoo is right in that this is about promise of the API regarding errno modifications. I think it would make sense to make such promise and keep errno untouched on return.
Since this is going to be changed in followup and errno=0 will be removed I think it's fine as is for this PR.

[stolowski]

+1 (assuming a followup PR introduces the changes about die_with_errno and avoids resetting errno).

[stolowski]

My main concern is obviously cases where sc_error_initv(..) is used to create and initialize sc_error, but after calling it we want to do something extra based on last errno value; and yes @bboozzoo is right in that this is about promise of the API regarding errno modifications. I think it would make sense to make such promise and keep errno untouched on return.
Since this is going to be changed in followup and errno=0 will be removed I think it's fine as is for this PR.