[openssl-dev] [openssl.org #3895] fprintf in ssl library

[Rich Salz via RT]

Summarizing some email from the team-internal thread.


> rsalz>
> rsalz> In record/ssl3_record.c:
> rsalz> fprintf(stderr,
> rsalz> "%s:%d: rec->data != rec->input\n",
> rsalz> __FILE__, __LINE__);
>
> From the comment above it, I'd say it's elligible to an abort() (I
> thoroughly hate abort() and avoid it like the plague, but there are
> times when I have to admit its usefulness):
>
> /*
> * we can't write into the input stream: Can this ever
> * happen?? (steve)
> */
>
drH: That code came with the initial DTLS stuff and I wasn't sure if that
condition could ever happen, hence the comment from way back. If it does then
the explicit IV handling wont work IIRC.

> rsalz> In s3_srvr.c:
> rsalz> if (i != 64) {
> rsalz> fprintf(stderr, "GOST signature length is %d", i);
> rsalz> }
>
> This looks weird to me. The code following this seems to assume a 64
> byte signature, BUT the comment around line 2916 suggests that a GOST
> signature can have other lengths as well. That suggests that this
> fprintf() is a debugging print... However, it does look to me like
> we're still only handling 64-byte long GOST signatures, so something
> isn't quite complete.
> ... and I need to read up on GOST.
>
> rsalz> In d1_both.c
> rsalz> if (code > 0) {
> rsalz> fprintf(stderr, "invalid state reached %s:%d", __FILE__, __LINE__);
> rsalz> return 1;
> rsalz> }
>
> This is in dtls1_read_failed(), which doesn't seem to be used anywhere...

Matt: It *is* used. It's called from the record layer. Actually this function
should probably be *in* the record layer since its only used from there.

The instance above is a "should never happen" scenario. We should probably just
log it as an internal error and remove the fprintf.
Probably the return code should be 0 not 1.

>
> rsalz> &found) <= 0 && found) {
> rsalz> fprintf(stderr, "dtls1_retransmit_message() failed\n");
> rsalz> return -1;
> rsalz> }

Matt: This could actually occur. But I think the correct thing to do is just
ignore the error, i.e. delete the fprintf altogether (and continue to return
-1).

>
> rsalz> if (item == NULL) {
> rsalz> fprintf(stderr, "retransmit: message %d non-existant\n", seq);
> rsalz> *found = 0;
> rsalz> return 0;
> rsalz> }
>

Matt: This one is another "should never happen". Log it as an internal error
and remove the fprintf.