Chttpd request processing does not reset pdict predictably #4909
Description
Description
The Mochiweb http server uses a shared and re-used connection pool with chttpd:handle_request as the entry point for the Mochiweb request processing handoff.
The bug is that we don't start with a fresh process dictionary between requests, as the chttpd processes are reused so pdict values that are not expressly cleared will persist. Even worse, pdict values that are only set by specific code paths will persist until the next time an incoming request makes its way to this particular chttpd worker in the pool, at which point the old values will be updated.
An analogy would be if we used paper receipt to log each request, but instead of starting with a fresh form every time, we re-use the old form but only erase the old values when we need to modify them, so any other form entries would persist from whatever prior request filled them.
We also don't set the nonce value until
couchdb/src/chttpd/src/chttpd.erl
Line 317 in 4e0b66a
nonce value will re-use the previously generated nonce value and return that as the "unique" value of the request, and will continue to return the existing value until a request is processed that reaches that line of code to generate a new nonce.
I propose we clear the pdict between requests. I don't think it's safe to do erlang:erase() as that'll also clear out the Mochiweb pdict values from https://github.com/apache/couchdb-mochiweb/blob/main/src/mochiweb_request.erl#L64-L78 so instead I propose a filtered clearing of the pdict values, like so:
diff --git a/src/chttpd/src/chttpd.erl b/src/chttpd/src/chttpd.erl
index ab8e1e9a3..ca55a8ac8 100644
--- a/src/chttpd/src/chttpd.erl
+++ b/src/chttpd/src/chttpd.erl
@@ -221,6 +221,7 @@ stop() ->
mochiweb_http:stop(?MODULE).
handle_request(MochiReq0) ->
+ couch_util:clear_pdict(), %% Make sure we start clean, everytime
erlang:put(?REWRITE_COUNT, 0),
MochiReq = couch_httpd_vhost:dispatch_host(MochiReq0),
handle_request_int(MochiReq).
diff --git a/src/couch/src/couch_util.erl b/src/couch/src/couch_util.erl
index 739df28e5..0bc9a94ba 100644
--- a/src/couch/src/couch_util.erl
+++ b/src/couch/src/couch_util.erl
@@ -46,6 +46,7 @@
-export([verify_hash_names/2]).
-export([get_config_hash_algorithms/0]).
-export([remove_sensitive_data/1]).
+-export([clear_pdict/0, clear_pdict/1]).
-include_lib("couch/include/couch_db.hrl").
@@ -870,3 +871,31 @@ remove_sensitive_data(KVList) ->
KVList1 = lists:keyreplace(<<"password">>, 1, KVList, {<<"password">>, <<"****">>}),
% some KVList entries are atoms, so test fo this too
lists:keyreplace(password, 1, KVList1, {password, <<"****">>}).
+
+-spec clear_pdict() -> ok.
+clear_pdict() ->
+ clear_pdict(erlang:get()).
+
+%% Exclude mochiweb markers, otherwise just use erlang:erase/0
+-spec clear_pdict(list()) -> ok.
+clear_pdict([]) ->
+ ok;
+clear_pdict([{mochiweb_request_body, _V} | Rest]) ->
+ clear_pdict(Rest);
+clear_pdict([{mochiweb_request_body_length, _V} | Rest]) ->
+ clear_pdict(Rest);
+clear_pdict([{mochiweb_request_cookie, _V} | Rest]) ->
+ clear_pdict(Rest);
+clear_pdict([{mochiweb_request_force_close, _V} | Rest]) ->
+ clear_pdict(Rest);
+clear_pdict([{mochiweb_request_path, _V} | Rest]) ->
+ clear_pdict(Rest);
+clear_pdict([{mochiweb_request_post, _V} | Rest]) ->
+ clear_pdict(Rest);
+clear_pdict([{mochiweb_request_qs, _V} | Rest]) ->
+ clear_pdict(Rest);
+clear_pdict([{mochiweb_request_recv, _V} | Rest]) ->
+ clear_pdict(Rest);
+clear_pdict([{Key, _V} | Rest]) ->
+ erlang:erase(Key),
+ clear_pdict(Rest).Where the skipped values are the pdict names from the linked Mochiweb code above. I'm not entirely sure about the legality nuances from copying in the Mochiweb names, so before I PR'ed that I figured it would be to hear back from @janl / @rnewson / etc on preferred approach here.
Alternatively, we could explicitly delete the values set but I really dislike how that results in any values falling through persisting for an unknown amount of time until it's cleared out by a similar request. So I proposed the "clear everything but Mochiweb entries" approach. Let me know what you think.