Skip to content

Commit f5cd21d

Browse files
edwintorokedwintorok
authored
Run static analysis on OCaml C stubs in the CI (xapi-project#6338)
Similar to the static analyzer introduced in xha, but for OCaml C stubs. This uses my https://ocaml.org/p/dune-compiledb/latest to product a `compile_commands.json` for the C stubs out of the `dune` rules (which can also be useful if you want to use `clangd` and get some editor integration about compiler warnings). This requires installing enough of the build dependencies to be able to run `dune` successfully (perhaps in the future that restriction can be removed). Caching is used though, and we only need to install the build deps when `dune` files change, otherwise we can reuse a cached `compile_commands.json`. Static analyzers, like CodeChecker support reading `compile_commands.json` and invoking static analyzers with the appropriate flags to preprocess, and analyze the C source code. We use `clang`, `clang-tidy` and `cppcheck` as the default analyzers, although more analyzers could be added in the future (CodeChecker supports converting `gcc -fanalyzer` output for example. GCC also supports emiting SARIF format directly, but github cannot parse it, because it doesn't implement the full SARIF spec). At the end we convert the results back to the standard SARIF format that Github also supports for its code scanning results, which will make it automatically add comments on PRs that introduce *new* bugs, without necessarily gating on them. I fixed some of the most obvious warnings, and suppressed some minor ones that we cannot fix (where the warning is caused by a Xen or OCaml header). More warnings can be skipped by adding to `.codechecker.json` if needed. So far it seems to have found a file descriptor leak in `unixpwd.c` on an error path, but I haven't gone through all the reports in detail yet.
2 parents 198b021 + 23e01d7 commit f5cd21d

File tree

7 files changed

+125
-9
lines changed

7 files changed

+125
-9
lines changed

‎.codechecker.json‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"analyze": [
3+
"--disable=misc-header-include-cycle",
4+
"--disable=clang-diagnostic-unused-parameter"
5+
]
6+
}

‎.github/workflows/codechecker.yml‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
name: Run CodeChecker static analyzer on XAPI's C stubs
2+
permissions: {}
3+
4+
on:
5+
push:
6+
pull_request:
7+
branches:
8+
- master
9+
- 'feature/**'
10+
- '*-lcm'
11+
12+
concurrency: # On new push, cancel old workflows from the same PR, branch or tag:
13+
group: ${{ github.workflow }}-${{github.event_name}}-${{ github.event.pull_request.number || github.ref }}
14+
cancel-in-progress: true
15+
16+
jobs:
17+
staticanalyzer:
18+
name: Static analyzer for OCaml C stubs
19+
runs-on: ubuntu-latest
20+
permissions:
21+
contents: read
22+
security-events: write
23+
env:
24+
XAPI_VERSION: "v0.0.0-${{ github.sha }}"
25+
26+
steps:
27+
- name: Checkout code
28+
uses: actions/checkout@v4
29+
30+
- name: Restore cache for compile_commands.json
31+
uses: actions/cache/restore@v4
32+
id: cache-cmds
33+
with:
34+
path: compile_commands.json
35+
key: compile_commands.json-v1-${{ hashFiles('**/dune') }}
36+
37+
- name: Setup XenAPI environment
38+
if: steps.cache-cmds.outputs.cache-hit != 'true'
39+
uses: ./.github/workflows/setup-xapi-environment
40+
with:
41+
xapi_version: ${{ env.XAPI_VERSION }}
42+
43+
- name: Install dune-compiledb to generate compile_commands.json
44+
if: steps.cache-cmds.outputs.cache-hit != 'true'
45+
run: |
46+
opam pin add -y ezjsonm https://github.com/mirage/ezjsonm/releases/download/v1.3.0/ezjsonm-1.3.0.tbz
47+
opam pin add -y dune-compiledb https://github.com/edwintorok/dune-compiledb/releases/download/0.6.0/dune-compiledb-0.6.0.tbz
48+
49+
- name: Trim dune cache
50+
if: steps.cache-cmds.outputs.cache-hit != 'true'
51+
run: opam exec -- dune cache trim --size=2GiB
52+
53+
- name: Generate compile_commands.json
54+
if: steps.cache-cmds.outputs.cache-hit != 'true'
55+
run: opam exec -- make compile_commands.json
56+
57+
- name: Save cache for cmds.json
58+
uses: actions/cache/save@v4
59+
with:
60+
path: compile_commands.json
61+
key: ${{ steps.cache-cmds.outputs.cache-primary-key }}
62+
63+
- name: Upload compile commands json
64+
uses: actions/upload-artifact@v4
65+
with:
66+
path: ${{ github.workspace }}/compile_commands.json
67+
68+
- uses: whisperity/codechecker-analysis-action@v1
69+
id: codechecker
70+
with:
71+
ctu: true
72+
logfile: ${{ github.workspace }}/compile_commands.json
73+
analyze-output: "codechecker_results"
74+
75+
- name: Upload CodeChecker report
76+
uses: actions/upload-artifact@v4
77+
with:
78+
name: codechecker_results
79+
path: "${{ steps.codechecker.outputs.result-html-dir }}"
80+
81+
# cppcheck even for other analyzers apparently, this is
82+
# codechecker's output
83+
- name: convert to SARIF
84+
shell: bash
85+
run: report-converter "codechecker_results" --type cppcheck --output codechecker.sarif --export sarif
86+
87+
- name: Upload CodeChecker SARIF report
88+
uses: actions/upload-artifact@v4
89+
with:
90+
name: codechecker_sarif
91+
path: codechecker.sarif
92+
93+
- name: Upload SARIF report
94+
uses: github/codeql-action/upload-sarif@v3
95+
with:
96+
sarif_file: codechecker.sarif

‎Makefile‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ JOBS = $(shell getconf _NPROCESSORS_ONLN)
66
PROFILE=release
77
OPTMANDIR ?= $(OPTDIR)/man/man1/
88

9-
.PHONY: build clean test doc python format install uninstall coverage
9+
.PHONY: build clean test doc python format install uninstall coverage analyze
1010

1111
# if we have XAPI_VERSION set then set it in dune-project so we use that version number instead of the one obtained from git
1212
# this is typically used when we're not building from a git repo
@@ -196,6 +196,17 @@ uninstall:
196196
dune uninstall $(DUNE_IU_PACKAGES3)
197197
dune uninstall $(DUNE_IU_PACKAGES4)
198198

199+
# An approximation, we actually depend on all dune files recursively
200+
# Also fixup the directory paths to remove _build
201+
# (we must refer to paths that exist in the repository for static analysis results)
202+
compile_commands.json: Makefile dune
203+
mkdir -p _build/
204+
dune rules | dune-compiledb -o _build/
205+
sed -e 's/"directory".*/"directory": ".",/' <_build/$@ >$@
206+
207+
analyze: compile_commands.json Makefile .codechecker.json
208+
CodeChecker check --config .codechecker.json -l compile_commands.json
209+
199210
compile_flags.txt: Makefile
200211
(ocamlc -config-var ocamlc_cflags;\
201212
ocamlc -config-var ocamlc_cppflags;\

‎ocaml/auth/xa_auth.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@
1111
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
1212
* GNU Lesser General Public License for more details.
1313
*/
14-
#ifndef _XA_AUTH_H_
15-
#define _XA_AUTH_H_
14+
#ifndef XA_AUTH_H_
15+
#define XA_AUTH_H_
1616

1717
#define XA_SUCCESS 0
1818
#define XA_ERR_EXTERNAL 1

‎ocaml/libs/log/syslog_stubs.c‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,12 @@
2020
#include <caml/custom.h>
2121
#include <caml/signals.h>
2222

23-
static int __syslog_level_table[] = {
23+
static int syslog_level_table[] = {
2424
LOG_EMERG, LOG_ALERT, LOG_CRIT, LOG_ERR, LOG_WARNING,
2525
LOG_NOTICE, LOG_INFO, LOG_DEBUG
2626
};
2727

28-
static int __syslog_facility_table[] = {
28+
static int syslog_facility_table[] = {
2929
LOG_AUTH, LOG_AUTHPRIV, LOG_CRON, LOG_DAEMON, LOG_FTP, LOG_KERN,
3030
LOG_LOCAL0, LOG_LOCAL1, LOG_LOCAL2, LOG_LOCAL3,
3131
LOG_LOCAL4, LOG_LOCAL5, LOG_LOCAL6, LOG_LOCAL7,
@@ -54,8 +54,8 @@ value stub_syslog(value facility, value level, value msg)
5454
{
5555
CAMLparam3(facility, level, msg);
5656
char *c_msg = strdup(String_val(msg));
57-
int c_facility = __syslog_facility_table[Int_val(facility)]
58-
| __syslog_level_table[Int_val(level)];
57+
int c_facility = syslog_facility_table[Int_val(facility)]
58+
| syslog_level_table[Int_val(level)];
5959

6060
caml_enter_blocking_section();
6161
syslog(c_facility, "%s", c_msg);

‎ocaml/libs/vhd/vhd_format_lwt/blkgetsize64_stubs.c‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,8 @@ CAMLprim value stub_blkgetsize64(value filename){
6969
}
7070
#endif
7171
close(fd);
72-
}
72+
} else
73+
size_in_bytes = -1;
7374
caml_leave_blocking_section();
7475
free((void*)filename_c);
7576

‎unixpwd/c/unixpwd.c‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ unixpwd_setspw(const char *user, char *password)
144144
*sp;
145145
char buf[BUFLEN];
146146
int tmp;
147-
FILE *tmp_file;
147+
FILE *tmp_file = NULL;
148148
char tmp_name[PATH_MAX];
149149
struct stat statbuf;
150150
int rc;
@@ -164,6 +164,8 @@ unixpwd_setspw(const char *user, char *password)
164164
return rc;
165165
}
166166
if (lckpwdf() != 0) {
167+
if (tmp_file)
168+
fclose(tmp_file);
167169
close(tmp);
168170
unlink(tmp_name);
169171
return ENOLCK;

0 commit comments

Comments
 (0)