Likely defect fix for issue #42. #46
Conversation
is not evhtp_type_client. Also checked for c->htp to not be null in a couple of ssl related functions.
- Fix issue with non-SNI vhost sets
* A few releases back, we replaced typed settings to a flag bitmask
There was some logic previously that did the following:
if (c->htp->parent && c->vhost_via_sni == 0)
Where c->vhost_via_sni is NOW `EVHTP_CONN_FLAG_VHOST_VIA_SNI` in
the flags mask.
Unfortunately, the logic above broke when it was converted to a
flag, and it causes the conditional to ONLY branch true if the virtual
host was set by SSL SNI instead of the reverse.
Simply put, the fix is: `!(conn->flags & EVHTP_CONN_FLAG_VHOST_VIA_SNI)`
(note the '!').
- Lot's of cleanup on readability and logic for htp__connection_writecb_
- Added some more detailed commentary for obscure conditionals.
- Use `evhtp_safe_free` for any free's in htp__connection_writecb_
NOTE: M<3D
|
Added 440e5b9 to your patch. Can you verify things still function the same? Seems to work fine for me. |
|
Let's get a good set of eyes to make sure everything here is covered properly (note we no longer assert on potential errors). I also found a logical flaw for virtual hosts due to swapping out variable fields into bitmasks in the last release. We need to verify the logic of https://github.com/criticalstack/libevhtp/blob/1.2.11n/evhtp.c#L1843 matches up with correctly with our bitmask changes. This little refactor is a result of giving issue #42 a closer analysis. |
NathanFrench
changed the title
|
|
||
| if (c->htp->parent && c->flags & EVHTP_CONN_FLAG_VHOST_VIA_SNI) | ||
| if (conn->type == evhtp_type_server) |
There was a problem hiding this comment.
Would it be appropriate to use evhtp_likely here, or is that pointless?
There was a problem hiding this comment.
I actually hate the idea that this is a connection tagged function - but references other types outside of the evhtp_connection_t struct.
I don't think it's needed here since there is not a false condition.
| @@ -2680,6 +2744,10 @@ htp__ssl_add_scache_ent_(evhtp_ssl_t * ssl, evhtp_ssl_sess_t * sess) | |||
| int slen; | |||
|
|
|||
| connection = (evhtp_connection_t *)SSL_get_app_data(ssl); | |||
| if (connection->htp == NULL) | |||
There was a problem hiding this comment.
Should evhtp_unlikely be used here given that this condition is unlikely to occur?
There was a problem hiding this comment.
I'm not even sure if that code is even used, to be honest :)
| @@ -2703,6 +2771,10 @@ htp__ssl_get_scache_ent_(evhtp_ssl_t * ssl, unsigned char * sid, int sid_len, in | |||
| evhtp_ssl_sess_t * sess; | |||
|
|
|||
| connection = (evhtp_connection_t * )SSL_get_app_data(ssl); | |||
| if (connection->htp == NULL) | |||
There was a problem hiding this comment.
As above. Should evhtp_unlikely be used here?
There was a problem hiding this comment.
Same as the other scache stuff; fairly certain it isn't even used. But I will doublecheck when I get home.
…nto bug-fix/issue42
Check connection->type to ensure that it is not evhtp_type_client. Also checked for c->htp to not be null in a couple of ssl related functions.