ci: Run analysis using CodeChecker by Swatinem · Pull Request #304 · getsentry/sentry-native

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

        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

Attention: Patch coverage is 53.12500% with 30 lines in your changes missing coverage. Please review.

Project coverage is 85.03%. Comparing base (30204bd) to head (ed26fdd).
Report is 409 commits behind head on master.

Additional details and impacted files

@@            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

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

        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

Uh oh!

Conversation

Swatinem commented Jun 17, 2020

Uh oh!

Swatinem commented Jun 22, 2020

Uh oh!

Swatinem Jun 22, 2020

Choose a reason for hiding this comment

Uh oh!

jan-auer Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

Swatinem Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

jan-auer Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flub Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jun 24, 2020 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jan-auer Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Jun 24, 2020 •

edited by codecov bot

Loading