I totally agree in the general case. BIO is a big pain, and it does seem crazy to use it for stdio.
However, in this specific case, this file already calls BIO_printf, BIO_puts and BIO_write to stdout and stderr, in an unchecked manner, several hundred times. I’m not sure if checking write() actually fixes the problem, as there may be no guarantees of IO ordering when mixing BIO_write, BIO_puts and regular write() calls to the same output stream. Might it be OK to consider unifying this single exception to the (bad) rule, and then removing all BIO to stdin/stdout all at once in a follow-up patch? On May 31, 2014, at 5:40 PM, Theo de Raadt <[email protected]> wrote: > Your change is wrong. > > There have been lots of discussion about trying to use intrinsics as > much as possible. Even though this is the openssl command, I think > the consideration is also valid here. > > You've just run into the reason for using intrinsics as much as > possible. BIO_write is different from write. See how insane this is? > > Anyways, the compiler is telling you to check the return value. > > Why don't you check it, and put in some effort determining what action > to take in that case? > > The result will be better. > > >> write() warns if its return value is unchecked. Replace with a BIO_write >> like all of the surrounding code uses anyway. >> --- >> src/apps/s_server.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/apps/s_server.c b/src/apps/s_server.c >> index 51f6b47..fb28489 100644 >> --- a/src/apps/s_server.c >> +++ b/src/apps/s_server.c >> @@ -1767,7 +1767,7 @@ sv_body(char *hostname, int s, unsigned char *context) >> i = SSL_read(con, (char *) buf, bufsize); >> switch (SSL_get_error(con, i)) { >> case SSL_ERROR_NONE: >> - write(fileno(stdout), buf, >> + BIO_write(bio_s_out, buf, >> (unsigned int) i); >> if (SSL_pending(con)) >> goto again; >> -- >> 1.9.3 >> >
