From 83129eeef50f6429d7cb46aa44006f14c064a224 Mon Sep 17 00:00:00 2001 From: Pietro Gagliardi Date: Sun, 9 Feb 2020 13:37:45 -0500 Subject: [PATCH] Normalized uiprivInternalError() calls to always try to fail gracefully, and make sure errors don't call abort() so we can resume when debugging (and because this isn't really our decision to make...). (The debugger breaks will be removed from release builds when we get to that point.) Finally, refined some TODOs and removed some stale ones (in this case, that were related to the first few things). --- common/main.c | 4 +++- darwin/main.m | 1 - haiku/main.cpp | 14 ++++++++------ sharedbits/alloc_impl.h | 8 ++++++-- sharedbits/strsafe_impl.h | 6 ++++-- test/errors.c | 2 +- unix/main.c | 1 - windows/main.cpp | 6 +++--- 8 files changed, 25 insertions(+), 17 deletions(-) diff --git a/common/main.c b/common/main.c index a0624761..ac3a56f2 100644 --- a/common/main.c +++ b/common/main.c @@ -30,8 +30,10 @@ bool uiprivInitReturnErrorf(uiInitError *err, const char *fmt, ...) va_start(ap, fmt); n = uiprivVsnprintf(err->Message, 256, fmt, ap); va_end(ap); - if (n < 0) + if (n < 0) { uiprivInternalError("encoding error returning initialization error; this means something is very very wrong with libui itself"); + abort(); // TODO handle this scenario more gracefully + } if (n >= 256) { // the formatted message is too long; truncate it err->Message[252] = '.'; diff --git a/darwin/main.m b/darwin/main.m index 8bc8a8bd..e26b1566 100644 --- a/darwin/main.m +++ b/darwin/main.m @@ -127,7 +127,6 @@ void uiprivReportError(const char *prefix, const char *msg, const char *suffix, [NSException raise:exceptionName format:@"%s: %s", prefix, msg]; debugBreak(); - abort(); // we shouldn't reach here } #ifdef __clang__ diff --git a/haiku/main.cpp b/haiku/main.cpp index ed5327bb..b610abdb 100644 --- a/haiku/main.cpp +++ b/haiku/main.cpp @@ -3,8 +3,7 @@ uiprivApplication *uiprivApp; -// TODO add format string warning detection to all these functions, where available -// TODO also see if we can convert this to a string, or use a known type for status_t instead of assuming it's int(32_t) +// TODO see if we can convert this to a string, or use a known type for status_t instead of assuming it's int(32_t) #define uiprivInitReturnStatus(err, msg, status) uiprivInitReturnErrorf(err, "%s: %ld", msg, status) static thread_id mainThread; @@ -52,8 +51,10 @@ void uiprivApplication::MessageReceived(BMessage *msg) case uiprivMsgQueueMain: status = msg->FindData("args", B_ANY_TYPE, &data, &size); - if (status != B_OK) + if (status != B_OK) { uiprivInternalError("BMessage::FindData() failed in uiprivApplication::MessageReceived() for uiQueueMain(): %ld", status); + return; + } args = (const struct queueMainArgs *) data; (*(args->f))(args->data); return; @@ -72,9 +73,11 @@ void uiprivSysQueueMain(void (*f)(void *data), void *data) msg = new BMessage(uiprivMsgQueueMain); status = msg->AddData("args", B_RAW_TYPE, &args, sizeof (struct queueMainArgs), true, 1); - if (status != B_OK) - // TODO decide if we should just give up in this case like we do with user errors + if (status != B_OK) { uiprivInternalError("BMessage::AddData() failed in uiQueueMain(): %ld", status); + delete msg; + return; + } status = uiprivApp->PostMessage(msg); // msg is copied by PostMessage() so we can delete it here delete msg; @@ -91,5 +94,4 @@ void uiprivReportError(const char *prefix, const char *msg, const char *suffix, { fprintf(stderr, "*** %s: %s. %s\n", prefix, msg, suffix); debugger("TODO"); - abort(); // we shouldn't reach here } diff --git a/sharedbits/alloc_impl.h b/sharedbits/alloc_impl.h index 6e6e3e05..e9bb2cac 100644 --- a/sharedbits/alloc_impl.h +++ b/sharedbits/alloc_impl.h @@ -13,8 +13,10 @@ void *sharedbitsPrefixName(Alloc)(size_t n, const char *what) void *p; p = malloc(n); - if (p == NULL) + if (p == NULL) { sharedbitsPrefixName(InternalError)("memory exhausted allocating %s", what); + abort(); // TODO handle more gracefully somehow + } memset(p, 0, n); return p; } @@ -22,8 +24,10 @@ void *sharedbitsPrefixName(Alloc)(size_t n, const char *what) void *sharedbitsPrefixName(Realloc)(void *p, size_t nOld, size_t nNew, const char *what) { p = realloc(p, nNew); - if (p == NULL) + if (p == NULL) { sharedbitsPrefixName(InternalError)("memory exhausted reallocating %s", what); + abort(); // TODO handle more gracefully somehow + } if (nNew > nOld) memset(((uint8_t *) p) + nOld, 0, nNew - nOld); return p; diff --git a/sharedbits/strsafe_impl.h b/sharedbits/strsafe_impl.h index d2b11693..b5276591 100644 --- a/sharedbits/strsafe_impl.h +++ b/sharedbits/strsafe_impl.h @@ -16,7 +16,7 @@ int sharedbitsPrefixName(Vsnprintf)(char *s, size_t n, const char *fmt, va_list // TODO figure out how to disambiguate between encoding errors (returns negative value; does not have documented errno values), other errors (returns negative value; errno == EINVAL), and truncations (returns -1; does not have documented errno values) ret = vsnprintf_s(s, n, _TRUNCATE, fmt, ap); if (ret == -1) - // TODO make this safe + // TODO make this safe (by having these functions return size_t? I forget now...) return (int) n; return ret; #else @@ -52,7 +52,7 @@ char *sharedbitsPrefixName(Strncpy)(char *dest, const char *src, size_t n) // because strncpy_s() doesn't do this memset(dest, '\0', n * sizeof (char)); err = strncpy_s(dest, n, src, _TRUNCATE); - if (err != 0 && err != STRUNCATE) + if (err != 0 && err != STRUNCATE) { // Yes folks, apparently strerror() is unsafe (it's not reentrant, but that's not the point of the MSVC security functions; that's about buffer overflows, and as you'll soon see there really is no need for what the "safe' version is given reentrancy concerns), and not only that, but the replacement, strerror_s(), requires copying and allocation! it's almost like they were TRYING to shove as many error conditions as possible in! // Oh, and you can't just use _sys_errlist[] to bypass this, because even that has a deprecation warning, telling you to use strerror() instead, which in turn sends you back to strerror_s()! // Of course, the fact _sys_errlist[] is a thing and that it's deprecated out of security and not reentrancy shows that the error strings returned by strerror()/strerror_s() are static and unchanging throughout the lifetime of the program, so a truly reentrant strerror_s() would just return the raw const string array directly, or a placeholder like "unknown error" otherwise, but that would be too easy! @@ -60,6 +60,8 @@ char *sharedbitsPrefixName(Strncpy)(char *dest, const char *src, size_t n) // (Furthermore, cppreference.com says there's strerrorlen_s(), but a) fuck C11, and b) MSDN does not concur.) // So, alas, you'll have to live with just having the error code; sorry. sharedbitsprivInternalError("error calling strncpy_s(): %d", err); + return 0; + } return dest; #else return strncpy(dest, src, n); diff --git a/test/errors.c b/test/errors.c index 36f30226..df0061b5 100644 --- a/test/errors.c +++ b/test/errors.c @@ -52,7 +52,7 @@ static void checkProgrammerErrorSubtestImpl(struct checkProgrammerErrorParams *p err = threadNewThread(checkProgrammerErrorThreadProc, p, &thread); if (err != 0) - // TODO these should only fatal out of the subtest + // TODO these should only fatal out of the subtest (see TODO below) TestFatalfFull(p->file, p->line, "error creating thread: " threadSysErrorFmt, threadSysErrorFmtArg(err)); err = threadThreadWaitAndFree(thread); if (err != 0) diff --git a/unix/main.c b/unix/main.c index 02099917..7c82465e 100644 --- a/unix/main.c +++ b/unix/main.c @@ -63,5 +63,4 @@ void uiprivReportError(const char *prefix, const char *msg, const char *suffix, { g_critical("%s: %s. %s", prefix, msg, suffix); G_BREAKPOINT(); - abort(); // we shouldn't reach here } diff --git a/windows/main.cpp b/windows/main.cpp index 46ccdbee..b930db73 100644 --- a/windows/main.cpp +++ b/windows/main.cpp @@ -42,7 +42,6 @@ static inline void setHInstance(void) ICC_DATE_CLASSES | /* date/time picker */ \ 0) -// TODO add format string warning detection to all these functions, where available #define uiprivInitReturnHRESULT(err, msg, hr) uiprivInitReturnErrorf(err, "%s: 0x%08I32X", msg, hr) static DWORD mainThread; @@ -106,8 +105,10 @@ void uiMain(void) hr = uiprivHrGetMessageW(&msg, NULL, 0, 0); if (hr == S_FALSE) // WM_QUIT return; - if (hr != S_OK) + if (hr != S_OK) { uiprivInternalError("GetMessageW() failed in uiMain(): 0x%08I32X", hr); + return; + } // TODO IsDialogMessage() TranslateMessage(&msg); DispatchMessageW(&msg); @@ -148,5 +149,4 @@ void uiprivReportError(const char *prefix, const char *msg, const char *suffix, OutputDebugStringA(suffix); OutputDebugStringA("\n"); DebugBreak(); - abort(); // we shouldn't reach here }