[SAST] Several minor unreachable code and redundant checks instances

[Canned-pineapple-8]

Running static analysis on uWSGI identified several minor issues in the codebase. These are low-priority findings with little to no runtime impact but they could be addressed during future code revisions.

1. Unreachable code in function uwsgi_read_response()
   File: /core/protocol.c

In the error handling branch (possible timeout):

uwsgi/core/protocol.c

Lines 30 to 46 in 8d116f7

	rlen = uwsgi_waitfd(fd, timeout);
	if (rlen > 0) {
	len = read(fd, ptr, remains);
	if (len <= 0)
	break;
	remains -= len;
	ptr += len;
	if (remains == 0) {
	ret = uh->modifier2;
	break;
	}
	continue;
	}
	// timed out ?
	else if (ret == 0)
	ret = -2;
	break;

Line 45 is never reached as ret always equals -1 at this point (it can only be reassigned in the if (rlen > 0) code block).

The same pattern appears later in the same function:

uwsgi/core/protocol.c

Lines 54 to 72 in 8d116f7

	ret = -1;
	while (remains > 0) {
	rlen = uwsgi_waitfd(fd, timeout);
	if (rlen > 0) {
	len = read(fd, ptr, remains);
	if (len <= 0)
	break;
	remains -= len;
	ptr += len;
	if (remains == 0) {
	ret = uh->modifier2;
	break;
	}
	continue;
	}
	// timed out ?
	else if (ret == 0)
	ret = -2;
	break;

Line 71 is unreachable.

I believe this is a copy-paste oversight. Perhaps
the condition should check rlen == 0 instead of ret == 0? It does, however, affect the function's return code so I believe fixing this would be beneficial.

2. Redundant UWSGI_AGAIN checks in uwsgi_response_write_headers_do(), uwsgi_response_sendfile_do_can_close()
   File: /core/writer.c

In function uwsgi_response_write_body_do() the code checks for UWSGI_AGAIN from uwsgi_response_write_headers_do():

uwsgi/core/writer.c

Lines 413 to 417 in 8d116f7

	int ret = uwsgi_response_write_headers_do(wsgi_req);
	if (ret == UWSGI_OK) goto sendbody;
	if (ret == UWSGI_AGAIN) return UWSGI_AGAIN;
	wsgi_req->write_errors++;
	return -1;

However, uwsgi_response_write_headers_do() never actually returns UWSGI_AGAIN, it's either UWSGI_OK or UWSGI_ERROR. The check for UWSGI_AGAIN is therefore redundant.
Possible fixes:

• remove the redundant check
• modify uwsgi_response_write_headers_do() so it actually returns UWSGI_AGAIN in the appropriate scenarios (though I'm not sufficiently familiar with the code to know if that's intended)

The same pattern appears below:

uwsgi/core/writer.c

Lines 503 to 507 in 8d116f7

	int ret = uwsgi_response_write_headers_do(wsgi_req);
	if (ret == UWSGI_OK) goto sendbody;
	if (ret == UWSGI_AGAIN) return UWSGI_AGAIN;
	wsgi_req->write_errors++;
	return -1;

And in uwsgi_response_sendfile_do_can_close():

uwsgi/core/writer.c

Lines 599 to 602 in 8d116f7

	int ret = uwsgi_response_write_headers_do(wsgi_req);
	if (ret == UWSGI_OK) goto sendfile;
	if (ret == UWSGI_AGAIN) return UWSGI_AGAIN;
	wsgi_req->write_errors++;

3. Unreachable code in uwsgi_sharedarea_init_keyval()
   File: /core/sharedarea.c

In the code below:

uwsgi/core/sharedarea.c

Lines 385 to 394 in 8d116f7

	if (pages) {
	if (fd > -1) {
	sa = uwsgi_sharedarea_init_fd(fd, len, offset);
	}
	else if (area) {
	sa = uwsgi_sharedarea_init_ptr(area, len);
	}
	else {
	sa = uwsgi_sharedarea_init(pages);
	}

Line 390 is unreachable. Variable area is initially set to NULL on line 368:

uwsgi/core/sharedarea.c

Line 368 in 8d116f7

char *area = NULL;

and never reassigned.

It seems likely that area was intended to be set somewhere around here, perhaps in an empty else if (s_ptr) block:

uwsgi/core/sharedarea.c

Lines 371 to 383 in 8d116f7

	int fd = -1;
	if (s_file) {
	fd = open(s_file, O_RDWR|O_SYNC);
	if (fd < 0) {
	uwsgi_error_open(s_file);
	exit(1);
	}
	}
	else if (s_fd) {
	fd = atoi(s_fd);
	}
	else if (s_ptr) {
	}

4. Redundant null check in uwsgi_websocket_handshake()
   File: core/websockets.c

In uwsgi_websocket_handshake(), sha1 is initialized as a stack-allocated buffer:

uwsgi/core/websockets.c

Line 420 in 8d116f7

char sha1[20];

And then is not modified up until passing to uwsgi_sha1_2n:

uwsgi/core/websockets.c

Lines 445 to 446 in 8d116f7

	// generate websockets sha1 and encode it to base64
	if (!uwsgi_sha1_2n(key, key_len, "258EAFA5-E914-47DA-95CA-C5AB0DC85B11", 36, sha1)) return -1;

uwsgi_sha1_2n writes the SHA1 digest into the provided buffer (sha1) and always returns the same pointer:

uwsgi/core/ssl.c

Lines 584 to 591 in 8d116f7

	char *uwsgi_sha1_2n(char *s1, size_t len1, char *s2, size_t len2, char *dst) {
	SHA_CTX sha;
	SHA1_Init(&sha);
	SHA1_Update(&sha, s1, len1);
	SHA1_Update(&sha, s2, len2);
	SHA1_Final((unsigned char *)dst, &sha);
	return dst;
	}

So it appears like checking the result for NULL is redundant here.

5. No-effect operator in uwsgi_commandline_config()
   File: /core/init.c

In the cycle below:

uwsgi/core/init.c

Lines 339 to 367 in 8d116f7

	if (optind < argc) {
	for (i = optind; i < argc; i++) {
	char *lazy = argv[i];
	if (lazy[0] != '[') {
	uwsgi_opt_load(NULL, lazy, NULL);
	// manage magic mountpoint
	int magic = 0;
	int j;
	for (j = 0; j < uwsgi.gp_cnt; j++) {
	if (uwsgi.gp[j]->magic) {
	if (uwsgi.gp[j]->magic(NULL, lazy)) {
	magic = 1;
	break;
	}
	}
	}
	if (!magic) {
	for (j = 0; j < 256; j++) {
	if (uwsgi.p[j]->magic) {
	if (uwsgi.p[j]->magic(NULL, lazy)) {
	magic = 1;
	break;
	}
	}
	}
	}
	}
	}
	}

The assignment magic = 1 on line 359 has no effect. The variable magic is reset to 0 on each loop iteration (line 345), so a plain break would be enough here.

6. Several double close() calls (uwsgi_alarm_func_cmd, emperor_check_on_demand_socket, uwsgi_scheme_exec)
   Files: core/alarm.c, core/emperor.c, core/io.c

Problem:
There is a recurring pattern (e.g. in uwsgi_alarm_func_cmd) where a file descriptor is closed twice - first inside a called function (uwsgi_run_command), then again in the caller. For example, the following code calls uwsgi_run_command:

uwsgi/core/alarm.c

Line 54 in 8d116f7

uwsgi_run_command(uai->data_ptr, pipe, -1);

The function forks the process and closes stdin (== pipe[0]) as the parent, then returns:

uwsgi/core/utils.c

Lines 2519 to 2537 in 8d116f7

	pid_t pid = fork();
	if (pid < 0) {
	return -1;
	}
	if (pid > 0) {
	if (stdin_fd && stdin_fd[0] > -1) {
	close(stdin_fd[0]);
	}
	if (stdout_fd > -1) {
	close(stdout_fd);
	}
	if (waitpid(pid, &waitpid_status, WNOHANG) < 0) {
	uwsgi_error("waitpid()");
	return -1;
	}
	return pid;
	}

Immediately after returning, the caller closes pipe[0] again:

uwsgi/core/alarm.c

Line 54 in 8d116f7

uwsgi_run_command(uai->data_ptr, pipe, -1);

Additional examples:
In emperor_check_on_demand_socket:

uwsgi/core/emperor.c

Lines 398 to 407 in 8d116f7

	int r = uwsgi_run_command(cmd, NULL, cpipe[1]);
	free(cmd);
	if (r < 0) {
	close(cpipe[0]);
	close(cpipe[1]);
	return NULL;
	}
	char *ret = uwsgi_read_fd(cpipe[0], &len, 1);
	close(cpipe[0]);
	close(cpipe[1]);

In uwsgi_scheme_exec:

uwsgi/core/io.c

Lines 130 to 133 in 8d116f7

	uwsgi_run_command(url, NULL, cpipe[1]);
	char *buffer = uwsgi_read_fd(cpipe[0], size, add_zero);
	close(cpipe[0]);
	close(cpipe[1]);

Suggested patch:
It won't cause any trouble in practice, the second close() will simply fail, but redundant calls could be removed to silence static analyzers.

These are very minor inconsistencies that are more style-related that actual bugs. Still, reporting them in case you ever decide to do some code cleanup in the future.

Found by Linux Verification Center with SVACE