cloudflare / keyless

Cloudflare's Keyless SSL Server Reference Implementation
Other
276 stars 78 forks source link

Fixed bug of processing pipelined keyless requests. #69

Closed cdnsec closed 10 years ago

cdnsec commented 10 years ago

This bug does not appear currently since keyless proxy initiates a new connection for each request, but it will be a problem once keyless proxy uses persistent connection.

jgrahamc commented 10 years ago

Can you go into a bit more detail about this?

The testclient already performs multiple requests per connection and so we already test for the situation where a single connection is reused.

The 'return 1;' removed at line won't seem to make any difference because the while loop is guarded by 'state->need > 0' and that (now removed) 'return 1;' is only reached when state->need == 0. So leaving in or removing that 'return 1;' seems to make no difference.

cdnsec commented 10 years ago

The tests in testclient only reuse the connection sequentially (i.e. sequence of send && recv operation pairs), but do not try any unordered send and recv operations (e.g. send multiple request messages before doing recv). Such behavior does not happen in current implementation of keyless proxy, but may appear if keyless proxy uses persistent connection in the future.

Actually set_get_header_state will reset state->need to the header size.

jgrahamc commented 10 years ago

OK. Now I understand what you are trying to achieve. Makes total sense. (And you are right about state->need).

Can you modify testclient to support pipeline requests and add tests for this functionality?

cdnsec commented 10 years ago

OK, I will add pipeline request tests into testclient.

PiotrSikora commented 10 years ago

@jgrahamc, @grittygrease: could you guys please take a look at this? The first 2 commits look good to me and they fix the issue I'm seeing with the edge not receiving some responses, which leads to corrupted persistent connection.

grittygrease commented 10 years ago

This looks good to me. @jgrahamc go ahead and merge if you're ok with this.

jgrahamc commented 10 years ago

One final question: what was the SIGPIPE handler added for?