WIP: ci: enable infer on test phase

infer is a static analysis tool from Facebook, that's enabled as a CI test to catch most common issues such as:

• stack array overflows
• possible NULL dereferences
• possible undefined behavior
• using garbage results

it's running with relaxed rules to prevent failing on false positives, (e.g. invalid memory leaks) and harmless errors (dead stores)

Let me know if you'd consider this, I'll fix the remaining bugs before merging. I disabled leak checker because it can produce false positives (https://github.com/facebook/infer/issues/8). I also disabled liveness checker because there's a lot of harmless dead stores (initializing variables for posterity), but maybe it'd be a good idea to fix this and keep it as it helps to find out dead code after changes.

modules/stats/stats.c:246: error: NULL_DEREFERENCE
  pointer `pair` last assigned on line 245 could be null and is dereferenced by call to `strchr()` at line 246, column 14.
  244.   	struct stat_data *data = module->data;
  245.   	auto_free char *pair = strdup(args);
  246. > 	char *val = strchr(pair, ' ');
  247.   	if (val) {
  248.   		*val = '\0';

daemon/engine.c:270: error: NULL_DEREFERENCE
  pointer `engine` last assigned on line 255 could be null and is dereferenced by call to `engine_get_moduledir()` at line 270, column 20.
  268.   	}
  269.
  270. > 	lua_pushstring(L, engine_get_moduledir(engine));
  271.   	return 1;
  272.   }

daemon/network.c:315: error: NULL_DEREFERENCE
  pointer `ep` last assigned on line 314 could be null and is dereferenced by call to `memset()` at line 315, column 2.
  313.   	/* Bind interfaces */
  314.   	struct endpoint *ep = malloc(sizeof(*ep));
  315. > 	memset(ep, 0, sizeof(*ep));
  316.   	ep->flags = NET_DOWN;
  317.   	ep->port = port;

client/kresc.c:321: error: USE_AFTER_FREE
  pointer `fd` last assigned on line 300 was freed by call to `fdopen()` at line 318, column 10 and is dereferenced or freed at line 321, column 3.
  319.   	if (!g_tty) {
  320.   		perror("While opening TTY");
  321. > 		close(fd);
  322.   		return 1;
  323.   	}

lib/cache.c:326: error: NULL_DEREFERENCE
  pointer `entry` last assigned on line 320 could be null and is dereferenced at line 326, column 11.
  324.   	}
  325.   	if (rank) {
  326. > 		*rank = entry->rank;
  327.   	}
  328.   	if (flags) {

lib/cache.c:329: error: NULL_DEREFERENCE
  pointer `entry` last assigned on line 320 could be null and is dereferenced at line 329, column 12.
  327.   	}
  328.   	if (flags) {
  329. > 		*flags = entry->flags;
  330.   	}
  331.   	rr->rrs.rr_count = entry->count;

lib/cache.c:331: error: NULL_DEREFERENCE
  pointer `entry` last assigned on line 320 could be null and is dereferenced at line 331, column 21.
  329.   		*flags = entry->flags;
  330.   	}
  331. > 	rr->rrs.rr_count = entry->count;
  332.   	rr->rrs.data = entry->data;
  333.   	return kr_ok();

lib/cache.c:371: error: NULL_DEREFERENCE
  pointer `valid` last assigned on line 366 could be null and is dereferenced at line 371, column 4.
  369.   	for (uint16_t i = 0; i < src->rrs.rr_count; ++i) {
  370.   		if (knot_rdata_ttl(rd) >= drift) {
  371. > 			valid[valid_count++] = rd;
  372.   		}
  373.   		rd = kr_rdataset_next(rd);

lib/utils.c:401: error: NULL_DEREFERENCE
  pointer `addr_str` last assigned on line 400 could be null and is dereferenced by call to `strchr()` at line 401, column 17.
  399.   	int family = kr_straddr_family(addr);
  400.   	auto_free char *addr_str = strdup(addr);
  401. > 	char *subnet = strchr(addr_str, '/');
  402.   	if (subnet) {
  403.   		*subnet = '\0';

daemon/main.c:462: error: NULL_DEREFERENCE
  pointer `addr_set.at` last assigned on line 430 could be null and is dereferenced at line 462, column 4.
  460.   		{
  461.   		case 'a':
  462. > 			array_push(addr_set, optarg);
  463.   			break;
  464.   		case 't':

daemon/main.c:468: error: NULL_DEREFERENCE
  pointer `fd_set.at` last assigned on line 433 could be null and is dereferenced at line 468, column 4.
  466.   			break;
  467.   		case 'S':
  468. > 			array_push(fd_set,  atoi(optarg));
  469.   			break;
  470.   		case 'T':

daemon/main.c:471: error: NULL_DEREFERENCE
  pointer `tls_fd_set.at` last assigned on line 435 could be null and is dereferenced at line 471, column 4.
  469.   			break;
  470.   		case 'T':
  471. > 			array_push(tls_fd_set,  atoi(optarg));
  472.   			break;
  473.   		case 'c':

daemon/main.c:465: error: NULL_DEREFERENCE
  pointer `tls_set.at` last assigned on line 431 could be null and is dereferenced at line 465, column 4.
  463.   			break;
  464.   		case 't':
  465. > 			array_push(tls_set, optarg);
  466.   			break;
  467.   		case 'S':

lib/dnssec/nsec3.c:501: error: NULL_DEREFERENCE
  pointer `encloser` last assigned on line 494 could be null and is dereferenced by call to `covers_closest_encloser_wildcard()` at line 501, column 8.
  499.   		return ret;
  500.   	}
  501. > 	ret = covers_closest_encloser_wildcard(pkt, section_id, encloser);
  502.   	if (ret != 0) {
  503.   		/* OK, but NSEC3 for wildcard at encloser has opt-out;

Summary of the reports

  NULL_DEREFERENCE: 13
    USE_AFTER_FREE: 1

Hmm, I was planning to add scan-build from Clang. What advantage Infer has over Clang scan-build? (I would rather avoid pulling random blobs into the image, but I can be persuaded if there are clear advantages.)

You should use both, they have different checkers and find different bugs.

The main reasons I like infer in CI is:

• it has less false positives or harmless reports - clang static analyzer is harder to configure to be suitable for failing builds without being obnoxious
• it supports differential scans - so you can only show errors between source and target branch

The static analyzer is more convenient at generating analysis reports that you can read afterwards, not as an automated QA.

added 26 commits

• 55e6016c...1286086c - 25 commits from branch master
• 743d3ed1 - ci: enable infer on test phase

Compare with previous version

assigned to @vavrusam

marked as a Work In Progress

changed the description

added tests label

Apparently unpacking tarball is not good enough, infer needs some build steps. Please modify Dockerfile to do build or close this MR, depends on how much time you want to spend on this.

Thank you!

I think it'd be more valuable to start fixing more stuff found by clang-tidy and CSA than adding more checkers, let me decline it for now.

closed

mentioned in commit fa498d31