We don't tolerate declarations in the middle of code. I mean in this case hvers would have to be declared (but not assigned) prior if (msglen). Secondly it looks like you have to compare to 3, not 2. Indeed it's msg[2]. However, if one actually aims to ensure that there is not overstepping, then shouldn't one have to perform more checks even further down?

Indeed, pushing a new version of the commit

While we are at it, one can wonder if it would be appropriate to check even for overlap. I mean suggested modifications try to ensure that there is no buffer overstepping by asserting upper limit for message length. And question is whether or not it would be appropriate to assert even lower limit. Disregard SSL_IS_DTLS for a moment, and imagine that msglen is permitted[!] 3. In such case length field will overlap version field. Would it be appropriate to check for this? Well, it might happen that these conditions will always be favourable, even upper limit check, because this function might be called with data that is vetted already. But if you start playing the game, then maybe one should play it wholeheartedly...

Interesting.
I guess that we want to avoid overlap (even if it is not an over read).
Anyone voting up for this solution ?

On side note! (And side note means that no action is expected/required.) Original condition was not actually wrong, because len is verified to be even. With this in mind I'd say that while (len >= 2) would be more readable. One can even emphasize it with /* len is always even */.

I did not see this check, so I guess I should remove this patch.
There are there loops with that kind of check.

We don't tolerate C++-style comments.

If you consider overall purpose of this function, you'll have to conclude that it's debugging. And debugging by examining output, not single-stepping. Question is if just breaking (and consequently omitting emitting empty line) is something that would help debugging? Wouldn't user appreciate to note bad things happening (just as much as good ones)? And yes, if taken to the logical conclusion you'd also have to verify that encoded payload length matches (or doesn't exceed?) msglen. Once again, wholeheartedly...

[On side note there seems to be inconsistency on how lines are terminated. They all end with \n, except for two...]

Did I add the right debug messages in the next patch ?

I don't think that you've got indentation right. I mean you used 4, but I don't think it's right. Though to be honest I don't actually know what's right, but it looks like it would be appropriate to align it with what applies in case in question, and it seems to be ... 0. But it's kind of odd that you can get "received" followed by "too short" in DTLS case. I'd say that following would be more appropriate:

int is_dtls = SSL_IS_DTLS(ssl);

BIO_puts(bio, write_p ? "Sent" : "Received");
if (msglen < is_dtls ? 13 : 5) {
    ssl_print_hex(bio, 1, "too short message", msg, msglen);
    break;
}
...
if (is_dtls) ...

Direction is meaningful and presumably valuable even in "too short message" case. BTW, while you are at it, it's suggested to update coding style guide and say that declarations should be delimited by empty line. In other words could you add an empty line after "int hvers"? :-)

is this correct ? doesn't that evaluate to ( msglen < .... ) ? 13 : 5 ???

yes, indeed using recent gcc it triggers the following warning:

ssl/t1_trce.c: In function 'SSL_trace':
ssl/t1_trce.c:1503:48: error: ?: using integer constants in boolean context, the expression will always evaluate to 'true' [-Werror=int-in-bool-context]
             if (msglen < SSL_IS_DTLS(ssl) ? 13 : 5) {
                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~

And as the author of Wint-in-bool-context I may say it makes me a bit proud!

It is! Keen eye :-)

yes, indeed using recent gcc it triggers the following warning:

ssl/t1_trce.c: In function 'SSL_trace':
ssl/t1_trce.c:1503:48: error: ?: using integer constants in boolean context, the expression will always evaluate to 'true' [-Werror=int-in-bool-context]
             if (msglen < SSL_IS_DTLS(ssl) ? 13 : 5) {
                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~

And as the author of Wint-in-bool-context I may say it makes me a bit proud!