fix: introduce malloc/MAKE rv checks if missing#1234
Conversation
supervacuus
force-pushed
the
fix/check_malloc_and_make_returns
branch
from
a2a8e2c to
73816a2
Compare
…oc return paths that propagate
supervacuus
force-pushed
the
fix/check_malloc_and_make_returns
branch
from
73816a2 to
7689a0c
Compare
|
@JoshuaMoelans, I started work on a While in practice, it is very rare that However, it highlights a more significant phenomenon, which I would like to have a more basic support for: checking returned pointers at call sites (which can be |
JoshuaMoelans
left a comment
JoshuaMoelans
left a comment
There was a problem hiding this comment.
This looks good, I just have one remark on a success/fail return value & two questions about applying the test changes more broadly.
| if (!libname) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
(from the function's docstring above) why is a null-libname a success?
/* Returns 1 on failure and 0 on success. "s" is the address of the symbol,
* and "i" points to a Dl_info structure to fill. Note that i.dli_fname is
* not const, and should be freed.
*/There was a problem hiding this comment.
Tbh, I didn't look at the inline docs. I examined the surrounding code, where every early exit returns 0, and the only success case I could find returns 1. Maybe I misread?
There was a problem hiding this comment.
Seems like the docstring is just wrong, and it returns 1 on success and 0 on early exit/failure, so your addition is functionally correct 👌
| for (size_t i = 0; i < 100; i++) { | ||
| sentry_bgworker_t *bgw = sentry__bgworker_new(NULL, NULL); | ||
| TEST_CHECK(!!bgw); | ||
| TEST_ASSERT(!!bgw); |
There was a problem hiding this comment.
q: Why do we only switch to TEST_ASSERT here? (Considering the docstring for it, aborting when no backgroundworker was created makes sense, but I just wonder if there might be more places where this change could be useful).
There was a problem hiding this comment.
Switching from CHECK to ASSERT generally means that continuing and aggregating errors could lead to a crash in the test itself, which is almost always much more complicated to pinpoint than a failed assertion.
TEST_ASSERTand similar macros should be used whenever further execution could lead to trouble.TEST_CHECKshould be used whenever a result is wrong, but the remaining test can still execute cleanly.
The latter is preferred because this way we get to fix all errors in one iteration. However, the former is a necessity for running tests safely.
| if (!libname) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Seems like the docstring is just wrong, and it returns 1 on success and 0 on early exit/failure, so your addition is functionally correct 👌
2 participants
#1224 raises the issue that there are still some unchecked returns from
SENTRY_MAKEandsentry_malloc. This PR (now) fixes this first layer, but we should also fix the internal uses of all functions affected by early returns. For instance, here:sentry-native/src/path/sentry_path.c
Lines 18 to 23 in 6a6e42f
path_itercould beNULL. Evaluating our coverage and keeping it would require a bit more work.#skip-changelog