Improve http_prepare_response()'s error semantics

I don't like the juggling with status-values in serve. It makes
sense for http_recv_header() and http_parse_header(), because we
don't have a response-struct yet that we can "fill". We could pass
it to them, but that would make the usage a bit messy.

However, in http_prepare_response(), we are already entrusted with
a pointer to a response-struct, and just failing here (by returning
an error value) leaves the response-struct in an invalid state. Instead,
we make it a void function and reflect the status using the status field
in the passed response struct.

This way, there is no case where the response struct is in an
invalid state after calling a http_prepare_*()-method.

Signed-off-by: Laslo Hunhold <dev@frign.de>

http.c

@@ -523,11 +523,11 @@ parse_range(const char *str, size_t size, size_t *lower, size_t *upper)
 #undef RELPATH
 #define RELPATH(x) ((!*(x) || !strcmp(x, "/")) ? "." : ((x) + 1))
 
-enum status
+void
 http_prepare_response(const struct request *req, struct response *res,
                       const struct server *srv)
 {
-	enum status returnstatus;
+	enum status s;
 	struct in6_addr addr;
 	struct stat st;
 	struct tm tm = { 0 };
@@ -544,7 +544,8 @@ http_prepare_response(const struct request *req, struct response *res,
 	/* make a working copy of the URI and normalize it */
 	memcpy(realuri, req->uri, sizeof(realuri));
 	if (normabspath(realuri)) {
-		return S_BAD_REQUEST;
+		s = S_BAD_REQUEST;
+		goto err;
 	}
 
 	/* match vhost */
@@ -559,13 +560,15 @@ http_prepare_response(const struct request *req, struct response *res,
 			}
 		}
 		if (i == srv->vhost_len) {
-			return S_NOT_FOUND;
+			s = S_NOT_FOUND;
+			goto err;
 		}
 
 		/* if we have a vhost prefix, prepend it to the URI */
 		if (vhost->prefix &&
 		    prepend(realuri, LEN(realuri), vhost->prefix)) {
-			return S_REQUEST_TOO_LARGE;
+			s = S_REQUEST_TOO_LARGE;
+			goto err;
 		}
 	}
 
@@ -583,7 +586,8 @@ http_prepare_response(const struct request *req, struct response *res,
 			/* swap out URI prefix */
 			memmove(realuri, realuri + len, strlen(realuri) + 1);
 			if (prepend(realuri, LEN(realuri), srv->map[i].to)) {
-				return S_REQUEST_TOO_LARGE;
+				s = S_REQUEST_TOO_LARGE;
+				goto err;
 			}
 			break;
 		}
@@ -591,19 +595,22 @@ http_prepare_response(const struct request *req, struct response *res,
 
 	/* normalize URI again, in case we introduced dirt */
 	if (normabspath(realuri)) {
-		return S_BAD_REQUEST;
+		s = S_BAD_REQUEST;
+		goto err;
 	}
 
 	/* stat the relative path derived from the URI */
 	if (stat(RELPATH(realuri), &st) < 0) {
-		return (errno == EACCES) ? S_FORBIDDEN : S_NOT_FOUND;
+		s = (errno == EACCES) ? S_FORBIDDEN : S_NOT_FOUND;
+		goto err;
 	}
 
 	if (S_ISDIR(st.st_mode)) {
 		/* append '/' to URI if not present */
 		len = strlen(realuri);
 		if (len + 1 + 1 > PATH_MAX) {
-			return S_REQUEST_TOO_LARGE;
+			s = S_REQUEST_TOO_LARGE;
+			goto err;
 		}
 		if (len > 0 && realuri[len - 1] != '/') {
 			realuri[len] = '/';
@@ -617,7 +624,8 @@ http_prepare_response(const struct request *req, struct response *res,
 	 */
 	if (strstr(realuri, "/.") && strncmp(realuri,
 	    "/.well-known/", sizeof("/.well-known/") - 1)) {
-		return S_FORBIDDEN;
+		s = S_FORBIDDEN;
+		goto err;
 	}
 
 	/*
@@ -646,7 +654,8 @@ http_prepare_response(const struct request *req, struct response *res,
 			 * honor that later when we fill the "Location"-field */
 			if ((ipv6host = inet_pton(AF_INET6, targethost,
 			                          &addr)) < 0) {
-				return S_INTERNAL_SERVER_ERROR;
+				s = S_INTERNAL_SERVER_ERROR;
+				goto err;
 			}
 
 			/* write location to response struct */
@@ -657,18 +666,20 @@ http_prepare_response(const struct request *req, struct response *res,
 			              targethost,
 			              ipv6host ? "]" : "", hasport ? ":" : "",
 			              hasport ? srv->port : "", tmpuri)) {
-				return S_REQUEST_TOO_LARGE;
+				s = S_REQUEST_TOO_LARGE;
+				goto err;
 			}
 		} else {
 			/* write relative redirection URI to response struct */
 			if (esnprintf(res->field[RES_LOCATION],
 			              sizeof(res->field[RES_LOCATION]),
 			              "%s", tmpuri)) {
-				return S_REQUEST_TOO_LARGE;
+				s = S_REQUEST_TOO_LARGE;
+				goto err;
 			}
 		}
 
-		return 0;
+		return;
 	} else {
 		/*
 		 * the URI is well-formed, we can now write the URI into
@@ -677,11 +688,13 @@ http_prepare_response(const struct request *req, struct response *res,
 		 * into the actual response-path
 		 */
 		if (esnprintf(res->uri, sizeof(res->uri), "%s", req->uri)) {
-			return S_REQUEST_TOO_LARGE;
+			s = S_REQUEST_TOO_LARGE;
+			goto err;
 		}
 		if (esnprintf(res->path, sizeof(res->path), "%s%s",
 		    vhost ? vhost->dir : "", RELPATH(req->uri))) {
-			return S_REQUEST_TOO_LARGE;
+			s = S_REQUEST_TOO_LARGE;
+			goto err;
 		}
 	}
 
@@ -692,7 +705,8 @@ http_prepare_response(const struct request *req, struct response *res,
 		 */
 		if (esnprintf(tmpuri, sizeof(tmpuri), "%s%s",
 		              req->uri, srv->docindex)) {
-			return S_REQUEST_TOO_LARGE;
+			s = S_REQUEST_TOO_LARGE;
+			goto err;
 		}
 
 		/* stat the docindex, which must be a regular file */
@@ -706,14 +720,16 @@ http_prepare_response(const struct request *req, struct response *res,
 				if (esnprintf(res->field[RES_CONTENT_TYPE],
 				              sizeof(res->field[RES_CONTENT_TYPE]),
 					      "%s", "text/html; charset=utf-8")) {
-					return S_INTERNAL_SERVER_ERROR;
+					s = S_INTERNAL_SERVER_ERROR;
+					goto err;
 				}
 
-				return 0;
+				return;
 			} else {
 				/* reject */
-				return (!S_ISREG(st.st_mode) || errno == EACCES) ?
+				s = (!S_ISREG(st.st_mode) || errno == EACCES) ?
-				       S_FORBIDDEN : S_NOT_FOUND;
+				    S_FORBIDDEN : S_NOT_FOUND;
+				goto err;
 			}
 		}
 	}
@@ -723,32 +739,33 @@ http_prepare_response(const struct request *req, struct response *res,
 		/* parse field */
 		if (!strptime(req->field[REQ_IF_MODIFIED_SINCE],
 		              "%a, %d %b %Y %T GMT", &tm)) {
-			return S_BAD_REQUEST;
+			s = S_BAD_REQUEST;
+			goto err;
 		}
 
 		/* compare with last modification date of the file */
 		if (difftime(st.st_mtim.tv_sec, timegm(&tm)) <= 0) {
 			res->status = S_NOT_MODIFIED;
-			return 0;
+			return;
 		}
 	}
 
 	/* range */
-	if ((returnstatus = parse_range(req->field[REQ_RANGE], st.st_size,
+	if ((s = parse_range(req->field[REQ_RANGE], st.st_size,
-	                                &(res->file.lower),
+	                     &(res->file.lower), &(res->file.upper)))) {
-	                                &(res->file.upper)))) {
+		if (s == S_RANGE_NOT_SATISFIABLE) {
-		if (returnstatus == S_RANGE_NOT_SATISFIABLE) {
 			res->status = S_RANGE_NOT_SATISFIABLE;
 
 			if (esnprintf(res->field[RES_CONTENT_RANGE],
 			              sizeof(res->field[RES_CONTENT_RANGE]),
 			              "bytes */%zu", st.st_size)) {
-				return S_INTERNAL_SERVER_ERROR;
+				s = S_INTERNAL_SERVER_ERROR;
+				goto err;
 			}
 
-			return 0;
+			return;
 		} else {
-			return returnstatus;
+			goto err;
 		}
 	}
 
@@ -774,34 +791,41 @@ http_prepare_response(const struct request *req, struct response *res,
 	if (esnprintf(res->field[RES_ACCEPT_RANGES],
 	              sizeof(res->field[RES_ACCEPT_RANGES]),
 		      "%s", "bytes")) {
-		return S_INTERNAL_SERVER_ERROR;
+		s = S_INTERNAL_SERVER_ERROR;
+		goto err;
 	}
 
 	if (esnprintf(res->field[RES_CONTENT_LENGTH],
 	              sizeof(res->field[RES_CONTENT_LENGTH]),
 	              "%zu", res->file.upper - res->file.lower + 1)) {
-		return S_INTERNAL_SERVER_ERROR;
+		s = S_INTERNAL_SERVER_ERROR;
+		goto err;
 	}
 	if (req->field[REQ_RANGE][0] != '\0') {
 		if (esnprintf(res->field[RES_CONTENT_RANGE],
 		              sizeof(res->field[RES_CONTENT_RANGE]),
 		              "bytes %zd-%zd/%zu", res->file.lower,
 			      res->file.upper, st.st_size)) {
-			return S_INTERNAL_SERVER_ERROR;
+			s = S_INTERNAL_SERVER_ERROR;
+			goto err;
 		}
 	}
 	if (esnprintf(res->field[RES_CONTENT_TYPE],
 	              sizeof(res->field[RES_CONTENT_TYPE]),
 	              "%s", mime)) {
-		return S_INTERNAL_SERVER_ERROR;
+		s = S_INTERNAL_SERVER_ERROR;
+		goto err;
 	}
 	if (timestamp(res->field[RES_LAST_MODIFIED],
 	              sizeof(res->field[RES_LAST_MODIFIED]),
 	              st.st_mtim.tv_sec)) {
-		return S_INTERNAL_SERVER_ERROR;
+		s = S_INTERNAL_SERVER_ERROR;
+		goto err;
 	}
 
-	return 0;
+	return;
+err:
+	http_prepare_error_response(req, res, s);
 }
 
 void

http.h

@@ -103,8 +103,8 @@ enum status http_send_header(int, const struct response *);
 enum status http_send_status(int, enum status);
 enum status http_recv_header(int, char *, size_t, size_t *);
 enum status http_parse_header(const char *, struct request *);
-enum status http_prepare_response(const struct request *, struct response *,
+void http_prepare_response(const struct request *, struct response *,
-                                  const struct server *);
+                           const struct server *);
 void http_prepare_error_response(const struct request *,
                                  struct response *, enum status);
 

main.c

@@ -39,9 +39,10 @@ serve(int infd, const struct sockaddr_storage *in_sa, const struct server *srv)
 
 	/* handle request */
 	if ((status = http_recv_header(c.fd, c.header, LEN(c.header), &c.off)) ||
-	    (status = http_parse_header(c.header, &c.req)) ||
+	    (status = http_parse_header(c.header, &c.req))) {
-	    (status = http_prepare_response(&c.req, &c.res, srv))) {
 		http_prepare_error_response(&c.req, &c.res, status);
+	} else {
+		http_prepare_response(&c.req, &c.res, srv);
 	}
 
 	status = http_send_header(c.fd, &c.res);