ci: Run analysis using CodeChecker #304

Swatinem · 2020-06-17T13:44:10Z

See https://github.com/Ericsson/codechecker

This basically is a frontend for clang-tidy and clang-static-analyzer.

Swatinem · 2020-06-22T16:16:45Z

Some examples of false positives I see with CTU running locally (had to disable that on CI for now):

[HIGH] /home/swatinem/Coding/sentry/sentry-native/src/sentry_string.c:39:5: Null pointer passed to 1st parameter expecting 'nonnull' [core.NonNullParamChecker]
    memcpy(sb->buf + sb->len, s, len);
    ^
  Report hash: bbf569e0c283fab9b521cb403de0903c
  Macro expansions:
    1, sentry_envelope.c:360:1: Macro 'MUST_USE' expanded to '__attribute__((warn_unused_result))'
  Steps:
     1, sentry_envelope.c:382:14: Calling 'sentry_envelope_write_to_path'
     2, sentry_envelope.c:360:1: Entered call from 'sentry_envelope_write_to_file'
     3, sentry_envelope.c:367:17: Calling 'sentry_envelope_serialize'
     4, sentry_envelope.c:348:1: Entered call from 'sentry_envelope_write_to_path'
     5, sentry_envelope.c:352:5: Calling 'sentry__stringbuilder_init'
     6, sentry_string.c:8:1: Entered call from 'sentry_envelope_serialize'
     7, sentry_string.c:11:5: Null pointer value stored to 'sb.buf'
     8, sentry_envelope.c:352:5: Returning from 'sentry__stringbuilder_init'
     9, sentry_envelope.c:354:5: Calling 'sentry__envelope_serialize_into_stringbuilder'
    10, sentry_envelope.c:291:1: Entered call from 'sentry_envelope_serialize'
    11, sentry_envelope.c:295:9: Assuming field 'is_raw' is true
    12, sentry_envelope.c:296:9: Calling 'sentry__stringbuilder_append_buf'
    13, sentry_string.c:53:1: Entered call from 'sentry__envelope_serialize_into_stringbuilder'
    14, sentry_string.c:57:12: Calling 'append'
    15, sentry_string.c:16:1: Entered call from 'sentry__stringbuilder_append_buf'
    16, sentry_string.c:20:9: Assuming 'needed' is <= field 'allocated'
    17, sentry_string.c:39:5: Null pointer passed to 1st parameter expecting 'nonnull'

I don’t think this can ever happen, as allocated will be 0 in that case; and needed is always >= 1 even if you want to append an empty string. Anyway, I will add another condition to it.

[MEDIUM] /home/swatinem/Coding/sentry/sentry-native/src/sentry_json.c:258:1: Potential leak of memory pointed to by 'formatted' [unix.Malloc]
}
^
  Report hash: d6367b9302465f90c90a3ab8eb9535cf
  Steps:
     1, sentry_json.c:255:23: Calling 'sentry__msec_time_to_iso8601'
     2, sentry_utils.c:327:1: Entered call from 'sentry__jsonwriter_write_msec_timestamp'
     3, sentry_utils.c:342:9: Assuming 'msecs' is 0
     4, sentry_utils.c:347:12: Calling 'sentry__string_clone'
     5, sentry_string.c:107:1: Entered call from 'sentry__msec_time_to_iso8601'
     6, sentry_string.c:110:18: Calling 'sentry__string_clonen'
     7, sentry_string.c:113:1: Entered call from 'sentry__string_clone'
     8, sentry_string.c:117:16: Calling 'sentry_malloc'
     9, sentry_alloc.c:13:1: Entered call from 'sentry__string_clonen'
    10, sentry_alloc.c:21:12: Memory is allocated
    11, sentry_string.c:117:16: Returned allocated memory
    12, sentry_string.c:118:9: Assuming 'rv' is non-null
    13, sentry_string.c:110:18: Returned allocated memory
    14, sentry_utils.c:347:12: Returned allocated memory
    15, sentry_json.c:255:23: Returned allocated memory
    16, sentry_json.c:258:1: Potential leak of memory pointed to by 'formatted'

Relevant code:

void
sentry__jsonwriter_write_msec_timestamp(sentry_jsonwriter_t *jw, uint64_t time)
{
    char *formatted = sentry__msec_time_to_iso8601(time);
    sentry__jsonwriter_write_str(jw, formatted);
    sentry_free(formatted);
}

lol, we just called sentry_free on it, how can that leak?

Looking at a lot more of these cases, I think the analyzer does not correctly handle ownership transfer of void*.
There are tons of false positives as well with sentry_path_free.

Swatinem · 2020-06-22T16:21:07Z

src/sentry_value.c

@@ -308,7 +308,11 @@ sentry_value_new_list(void)
    list_t *l = SENTRY_MAKE(list_t);
    if (l) {
        memset(l, 0, sizeof(list_t));
-        return new_thing_value(l, THING_TYPE_LIST);
+        sentry_value_t rv = new_thing_value(l, THING_TYPE_LIST);
+        if (sentry_value_is_null(rv)) {


These cases here are interesting. Sure, new_thing_value allocates internally and can fail. But CTU can’t look through the sentry_value_is_null assertion and will still flag all of these as false positives.

Have you checked if there are annotations that you can place sentry_value_is_null to make it understand the invariants? I'd love to take a look at this CTU failure.

The problem why I do all the checks outside is that new_thing_value takes ownership of whatever its wrapping, but it doesn’t know how to free. Maybe with a separate function pointer argument, this becomes clearer.

Usually, there are attributes you can put on the function to declare this behavior. Since we encode the pointers in a strange way, it's understandable that tools don't follow the path.

Let's skip this for now.

src/sentry_value.c

tests/cmake.py

flub · 2020-06-23T08:57:53Z

tests/cmake.py

+        errors = int(errors[: errors.find(b"\n")])
+        if errors > 0:
+            pytest.fail("code-checker analysis failed")
+


minor: personally this function is now too big and long for me to remember all variables in scope etc and i would break this up. at this point it's not problematic though, so i don't mind.

codecov-commenter · 2020-06-24T10:36:12Z

Codecov Report

Merging #304 into master will decrease coverage by 0.16%.
The diff coverage is 53.12%.

@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
- Coverage   85.20%   85.03%   -0.17%     
==========================================
  Files          49       49              
  Lines        3946     3975      +29     
==========================================
+ Hits         3362     3380      +18     
- Misses        584      595      +11

Impacted Files	Coverage Δ
src/modulefinder/sentry_modulefinder_linux.c	`90.99% <0.00%> (-0.29%)`	⬇️
src/sentry_session.c	`88.38% <0.00%> (-1.16%)`	⬇️
src/sentry_value.c	`84.15% <45.94%> (-0.62%)`	⬇️
src/sentry_core.c	`90.90% <50.00%> (-0.30%)`	⬇️
src/path/sentry_path_unix.c	`81.96% <66.66%> (-0.22%)`	⬇️
src/sentry_envelope.c	`87.76% <73.33%> (-0.22%)`	⬇️
src/sentry_string.c	`81.42% <100.00%> (ø)`
src/sentry_utils.c	`90.11% <100.00%> (+0.03%)`	⬆️
src/symbolizer/sentry_symbolizer_unix.c	`91.66% <0.00%> (-8.34%)`	⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30204bd...ed26fdd. Read the comment docs.

azure-pipelines.yml

src/path/sentry_path_unix.c

src/sentry_envelope.c

src/sentry_string.h

jan-auer · 2020-07-06T08:56:51Z

src/sentry_value.c

@@ -308,7 +308,11 @@ sentry_value_new_list(void)
    list_t *l = SENTRY_MAKE(list_t);
    if (l) {
        memset(l, 0, sizeof(list_t));
-        return new_thing_value(l, THING_TYPE_LIST);
+        sentry_value_t rv = new_thing_value(l, THING_TYPE_LIST);
+        if (sentry_value_is_null(rv)) {


Have you checked if there are annotations that you can place sentry_value_is_null to make it understand the invariants? I'd love to take a look at this CTU failure.

src/sentry_value.c

tests/__init__.py

Swatinem force-pushed the ci/scan-build branch 4 times, most recently from 9fb46ba to f6cb01d Compare June 17, 2020 16:13

Swatinem added 2 commits June 22, 2020 14:49

ci: Run analysis using CodeChecker

50fac7e

run code-checker with gcc on azure

a5ec832

Swatinem force-pushed the ci/scan-build branch from f6cb01d to a5ec832 Compare June 22, 2020 13:26

Swatinem added 2 commits June 22, 2020 15:49

disable CTU for now

bb3b017

fix two issues found by code-checker

3fcaa07

fix some potential issues identified by a CTU run

d1fc167

Swatinem commented Jun 22, 2020

View reviewed changes

Swatinem marked this pull request as ready for review June 22, 2020 16:29

Swatinem requested a review from a team June 22, 2020 16:29

flub approved these changes Jun 23, 2020

View reviewed changes

update codechecker python disables

75540c2

Swatinem mentioned this pull request Jun 24, 2020

test: Add more testcases for previously untested code #317

Merged

Merge branch 'master' into ci/scan-build

561844d

Swatinem added 2 commits June 24, 2020 13:20

fix relative db path, fix python chained exceptions

06237f0

Merge branch 'master' into ci/scan-build

025166f

Swatinem requested a review from jan-auer July 3, 2020 13:28

Merge branch 'master' into ci/scan-build

d074d1a

jan-auer reviewed Jul 6, 2020

View reviewed changes

Swatinem and others added 3 commits July 6, 2020 13:11

revert tolower change

d7df62f

half of the review feedback

c00832e

wording update

ed26fdd

jan-auer approved these changes Jul 6, 2020

View reviewed changes

Swatinem merged commit 4adf9c3 into master Jul 6, 2020

Swatinem deleted the ci/scan-build branch July 6, 2020 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: Run analysis using CodeChecker #304

ci: Run analysis using CodeChecker #304

Swatinem commented Jun 17, 2020

Swatinem commented Jun 22, 2020

Swatinem Jun 22, 2020

jan-auer Jul 6, 2020

Swatinem Jul 6, 2020

jan-auer Jul 6, 2020

flub Jun 23, 2020

codecov-commenter commented Jun 24, 2020 •

edited

Loading

jan-auer Jul 6, 2020

ci: Run analysis using CodeChecker #304

ci: Run analysis using CodeChecker #304

Conversation

Swatinem commented Jun 17, 2020

Swatinem commented Jun 22, 2020

Swatinem Jun 22, 2020

Choose a reason for hiding this comment

jan-auer Jul 6, 2020

Choose a reason for hiding this comment

Swatinem Jul 6, 2020

Choose a reason for hiding this comment

jan-auer Jul 6, 2020

Choose a reason for hiding this comment

flub Jun 23, 2020

Choose a reason for hiding this comment

codecov-commenter commented Jun 24, 2020 • edited Loading

Codecov Report

jan-auer Jul 6, 2020

Choose a reason for hiding this comment

codecov-commenter commented Jun 24, 2020 •

edited

Loading