i don't see it in the style, but we conventionally have a struct name here.

Done

don't need a length check on the incoming session_id?

No, we did it already when we first parsed it.

remove the else after return's

actually, can't you just do "return e1->type - e2->type" ?

return e1->type - e2->type isn't feasible. type is an unsigned int...

Removed the else.

blank line after declarations.

Done

I think "collect" is better than "parse_raw" or perhaps "parse_all" instead of "parse_raw"

Renamed to tls_collect_extensions()

indent to be under the opening parent?

The line is too long to do that...you'd have to put the == 0 on the next line. I thought it was more readable this way.

That's not what @richsalz meant, he meant this:

+                if (s->ctx->app_verify_cookie_cb(s, clienthello.dtls_cookie,
+                    clienthello.dtls_cookie_len) == 0) {

and I know you disagree with that style...

no, i meant line it up under the s,

curly in wrong spot.

Oops. Fixed.

why parens around ciphers?

It was there in the original code...but fixed.

you should introduce a local variable to factor out all the hello->pre_proc_exts[loop] expressions inside this loop.

and why isn't this a big switch statement?

and make the loop count down:

for (loop = hello->num_extensions; loop; --loop) {

That suggested for loop is just wrong, you would need to start at num_extensions - 1 or use loop - 1 everywhere inside the for loop. Note that starting from num_extensions - 1 will give you a problem because loop is a size_t. I also don't see why this should loop in the reverse order.

no, the loop is right. and if you have a local pointer variable like I also said, then you never need to refer to "loop" inside the body of the loop.

I used a local pointer variable - that makes a lot of sense. I haven't changed the loop to count down...I don't see why it makes any difference?

It's a micro optimization. On a machine instruction level, comparing with zero is a bit faster than comparing with another value. For a loop with a large span, it actually makes a difference. In this case, the difference is probably deeply marginal.

use a local pointer and make the loop count down:

for (loop = numexts; loop; --loop, ++ext) 
    if (exts->type == type)
       return ext;

Same as above. Why do you want count down? Is this some premature optimization thing?

it's just a habit of mine. sure, if you want to call it premature, fine. the key point is have a local pointer variable and always use that.

Not sure I see the point here? There are only two uses of loop here anyway...it doesn't seem to simplify things at all? Similarly I'm not sure why counting down is better.

In assembler it's usually faster to compare to 0 than to compare to some other number. In real applications you usually can't measure the difference.

And I think the compiler is actually smart enough to turn that into a pointer itself. And it might also do some other optimizations depending on in which direction you go. But I really don't see the point.

You could for intance also write it as:

for (ext = exts + numexts; ext != exts; ext--)
    if (ext->type == type)
        return ext;

and also, notice that there is actually no extra variable here. look at the code i wrote.

You mean the one that always looks at the same first entry?

Can we just agree to disagree on this?

well it was a comment :) but yeah, ext++ in the end of the for loop.
i'm advocating for a particular style, but i am not going to minus-one over it.

You forgot to update the documentation above.

Fixed.

Please document return values

Done

This looks like it's a largely based on tls1_check_duplicate_extensions(). Can we get rid of that function and use this new one for both the client and the server?

That was actually my thinking, but we can't do it just yet without also refactoring the ServerHello extension parsing too. That was more than I wanted to do in this PR at this stage. I've added a TODO item to remind us to come back to that and delete tls1_check_duplicate_extensions() later.

Can we maybe get rid of this client_version variable?

Hmmmm...the hello structure is currently transient and disappears once we've finished the ClientHello processing. We need client_version later though, so I don't think we can delete it just yet.

Please add some short documentation.

Done

Please document this function.

Done

You get the extensions now, not just the types.

Fixed.

I'm wondering if we still have a test that sends a v2 client hello (with TLS 1.0 for instance, like 0.9.8 used to do).

Yes we do. See 70-test_sslrecords.t

This documentation is outdated.

Updated.

I guess that is NULL when not found?

Yes...failure means not found...do you want me to change it to say that?

Yes

Done

blank line after the return.

Fixed

delete this blank line.

Fixed.

Per the discussion in #1771 should we consider moving SNI processing up here, or at least before ssl_get_prev_session()?
I had started on a WIP but got interrupted; there are some gnarly compatibility issues with respect to how currently the servername_callback can inspect the session (and the tlsext_hostname therein). I am not sure how well those issues can be resolved.

Maybe, but I've not been following #1771, so not as part of this PR.