apache / apache/couchdb

Chttpd request processing does not reset pdict predictably

Open
#4,909 7 comments 0 reactions 0 assignees View on GitHub
bug needs-triage
Dominant language
Erlang
Stars
7k
Forks
1.1k
Avg merge
1d 16h
Merged PRs (30d)
9

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 https://github.com/apache/couchdb/blob/4e0b66a4f8ad78388d9520306608cab3ef7581b7/src/chttpd/src/chttpd.erl#L317, nearly 100 lines of code into the request handling, which means that any runtime errors happening prior to the setting of the `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
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.

Contributor guide

Open the contributing guide

Research direction

Start at handle_request/1 in src/chttpd/src/chttpd.erl and inspect the process-dictionary entries used during request handling. Review src/couch/src/couch_util.erl and the listed Mochiweb markers in mochiweb_request.erl before deciding how cleanup should preserve them. Done means reused workers begin each request without stale application values while Mochiweb request state remains available.

Written by the indexing model from the issue text.

Assessment

Tech stack
erlang
Domain
api, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.