Commit Diff

Commit:
0e49c9bbcbfef05d5bdf9ad6d851f51fe25c2eaa
From:
Omar Polo <op@omarpolo.com>
Date:
Message:
gemini_parse_reply: return code and don't close connection clang static analyzer found a possible use after free: if the code is not 2X in gemini_parse_reply we call close_conn. Then, in net_read we set req->done_header, but req may have been free'd! Actually, this is almost impossible to trigger. close_conn never ends up calling free(req) on its first try, because tls_close takes a while and is rescheduled by libevent. (The check req->ctx != NULL in close_conn is always true if it's a gemini request.) Nevertheless, it's clear to move close_conn out of gemini_parse_reply and simply return the response code: it feels wrong that a "parsing function" takes logic decisions.
Actions:
Patch | Tree
commit - 3591e6dbd5cf94dae2b0a438542bf542ae094c01
commit + 0e49c9bbcbfef05d5bdf9ad6d851f51fe25c2eaa
blob - d07b0c84d2a79d85d34267721486841e9e4eda80
blob + 35e3674d618b8b5ad01dd54d45f883e9fde01359
--- net.c
+++ net.c
@@ -510,9 +510,7 @@ gemini_parse_reply(struct req *req, const char *header
 
 	bufferevent_disable(req->bev, EV_READ|EV_WRITE);
 
-	if (code < 20 || code >= 30)
-		close_conn(0, 0, req);
-	return 1;
+	return code;
 }
 
 /* called when we're ready to read/write */
@@ -565,9 +563,11 @@ net_read(struct bufferevent *bev, void *d)
 			return;
 		r = gemini_parse_reply(req, header, len);
 		free(header);
-		if (!r)
+		req->done_header = 1;
+		if (r == 0)
 			goto err;
-		req->done_header = 1;
+		else if (r < 20 || r >= 30)
+			close_conn(0, 0, req);
 		return;
 	}