On Tue, 2023-12-12 at 16:30 +0100, Siteshwar Vashisht wrote: [...snip...]
Hi Siteshwar, thanks for working on this. It looks like you're got the basic infrastructure of scanning working, but the prototype seems to be missing some things that IMHO would be essential to package maintainers actually using this, specifically: - easy-to-read results (so that a maintainer can easily answer the questions "what is the tool telling me?", "is this a real problem?", "what can/should I do about this?") - result management (e.g. "what issues have we seen in this past with this package?", "is this a flaky test that keeps coming and going?", etc) [FWIW I'm the upstream author/maintainer of the GCC static analyzer; I'm also on the SARIF technical committee.] > There are 3 different types of scans supported by OpenScanHub: > > - > > MockBuild performs a full scan of the package including downstream > patches. Example[8] mockbuild for `openssl-3.1.1-4.fc39`. Looking at a result from the GCC static analyzer e.g. this -Wanalyzer- deref-before-check: https://staging-openscanhub.apps.ocp.stg.fedoraproject.org/task/6/log/openssl-3.1.1-4.fc39/scan-results.html#def67 ...I see that the scan-results.html view quoted the pertinent source code (which is good), but unfortunately it only shows the summary message from the diagnostic: Error: GCC_ANALYZER_WARNING (CWE-465): [#def67] openssl-3.1.1/crypto/bn/bn_blind.c: scope_hint: In function 'BN_BLINDING_update' openssl-3.1.1/crypto/bn/bn_blind.c:108:12: warning[-Wanalyzer-deref-before-check]: check of 'b' for NULL after already dereferencing it # 106| !(b->flags & BN_BLINDING_NO_RECREATE)) { # 107| /* re-create blinding parameters */ # 108|-> if (!BN_BLINDING_create_param(b, NULL, NULL, ctx, NULL, NULL)) # 109| goto err; # 110| } else if (!(b->flags & BN_BLINDING_NO_UPDATE)) { But each diagnostic from the GCC analyzer has a list of events associated with it, which contain essential information for a human to understand the warning and evaluate it. For the example above, if I look at the raw log: https://staging-openscanhub.apps.ocp.stg.fedoraproject.org/task/6/log/openssl-3.1.1-4.fc39/scan.log ...I see: /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c: In function 'BN_BLINDING_update': <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:108:12: warning: check of 'b' for NULL after already dereferencing it [-Wanalyzer-deref-before-check] <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:93:5: note: (1) entry to 'BN_BLINDING_update' <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:97:11: note: (2) pointer 'b' is dereferenced here <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:97:8: note: (3) following 'false' branch... <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:102:10: note: (4) ...to here <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:105:8: note: (5) following 'true' branch... <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:108:14: note: (6) ...to here <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:108:14: note: (7) calling 'BN_BLINDING_create_param' from 'BN_BLINDING_update' <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:234:14: note: (8) entry to 'BN_BLINDING_create_param' <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:247:8: note: (9) following 'false' branch (when 'b' is non-NULL)... <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:255:12: note: (10) ...to here <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:255:8: note: (11) following 'false' branch... <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:257:12: note: (12) ...to here <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:257:8: note: (13) following 'false' branch... <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:260:8: note: (14) ...to here <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:260:8: note: (15) following 'false' branch (when 'e' is NULL)... <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:264:12: note: (16) ...to here <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:264:8: note: (17) following 'false' branch... <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:267:8: note: (18) ...to here <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:267:8: note: (19) following 'false' branch (when 'bn_mod_exp' is NULL)... <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:269:8: note: (20) ...to here <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:269:8: note: (21) following 'false' branch (when 'm_ctx' is NULL)... <--[gcc] cc1: note: (22) ...to here /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:307:8: note: (23) following 'false' branch (when 'b' is non-NULL)... <--[gcc] cc1: note: (24) ...to here /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:108:14: note: (25) returning to 'BN_BLINDING_update' from 'BN_BLINDING_create_param' <--[gcc] /builddir/build/BUILD/openssl-3.1.1/crypto/bn/bn_blind.c:108:12: note: (26) pointer 'b' is checked for NULL here but it was already dereferenced at (2) <--[gcc] where events (2) and (26) contain the most pertinent information (but without quoting the pertinent source code). I think this isn't going to be usable without some way for developers to view the chain of events, and for that view to show the source code at each event, so that it's trivial for the developer to review the above and answer the questions I mentioned above. I took a few minutes poking about on github trying to find the pertinent code to try to decide if the above was a true or false positive, and couldn't find the exact code, so I gave up. Also, I think it would make sense to use SARIF as a serialization format for capturing results from the analyzers, since it captures the information of interest, and it widely supported; see e.g. https://github.com/oasis-tcs/sarif-spec/wiki/Known-Producers-and-Consumers Doing so might help with result management. [...snip...] Hope this is constructive; thanks again for working on this. Dave -- _______________________________________________ devel mailing list -- devel@lists.fedoraproject.org To unsubscribe send an email to devel-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
