Return {error, max_length} instead of exception in cowboy_req:body/2 #290

Closed
benbro opened this Issue Oct 17, 2012 · 6 comments

3 participants

@benbro

When the request max length is larger than the limit, cowboy throws an exception error:function_clause.
Wouldn't it be better to catch this case and return {error, max_length} or something similar?

https://github.com/extend/cowboy/blob/master/src/cowboy_req.erl#L705

-spec read_body(non_neg_integer() | infinity, Req, binary())
    -> {ok, binary(), Req} | {error, atom()} when Req::req().
read_body(MaxLength, Req, Acc) when MaxLength > byte_size(Acc) ->
    case stream_body(Req) of
        {ok, Data, Req2} ->
            read_body(MaxLength, Req2, << Acc/binary, Data/binary >>);
        {done, Req2} ->
            {ok, Acc, Req2};
        {error, Reason} ->
            {error, Reason}
    end;
read_body(_MaxLength, _Req, _Acc)  -> {error, max_length}.
@benbro benbro referenced this issue Nov 5, 2012
Closed

read_body/2 bug? #309

@dvv
dvv commented Nov 5, 2012

seconded

@essen
Nine Nines member

Just wondering why you would have a MaxLength larger than the request body size though?

@dvv
dvv commented Dec 1, 2012

take a look at #309

@essen
Nine Nines member

No answers there.

@essen
Nine Nines member

I'm sorry to say that I'm thinking of removing this function entirely.

It's just too unsafe. Me and @dvv agree it's best handled in your own code, where you can choose to throw, or to handle it, by calling stream_body directly, and looping similarly to this. Problem is that Cowboy here has no way of doing it without introducing risks that you might end up with the wrong state depending on what you are doing with it.

I prefer removing and perhaps providing an example later on instead.

@essen
Nine Nines member

Removed in 2690d12. Thanks!

@essen essen closed this Dec 26, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment