Set rwstate consistently.
We reset it to SSL_NOTHING at the start of ever SSL_get_error-using operation.
Then we only set it to a non-NOTHING value in the rest of the stack on error
paths.
Currently, ssl->rwstate is set all over the place. Sometimes the pattern is:
ssl->rwstate = SSL_WRITING;
if (BIO_write(...) <= 0) {
goto err;
}
ssl->rwstate = SSL_NOTHING;
Sometimes we only set it to the non-NOTHING value on error.
if (BIO_write(...) <= 0) {
ssl->rwstate = SSL_WRITING;
}
ssl->rwstate = SSL_NOTHING;
Sometimes we just set it to SSL_NOTHING far from any callback in random places.
The third case is arbitrary and clearly should be removed.
But, in the second case, we sometimes forget to undo it afterwards. This is
largely harmless since an error in the error queue overrides rwstate, but we
don't always put something in the error queue (falling back to
SSL_ERROR_SYSCALL for "I'm not sure why it failed. Perhaps it was one of your
callbacks? Check your errno equivalent."), but in that case a stray rwstate
value will cause it to be wrong.
We could fix the cases where we fail to set SSL_NOTHING on success cases, but
this doesn't account for there being multiple SSL_get_error operations. The
consumer may have an SSL_read and an SSL_write running concurrently. Instead,
it seems the best option is to lift the SSL_NOTHING reset to the operations and
set SSL_WRITING and friends as in the second case.
(Someday hopefully we can fix this to just be an enum that is internally
returned. It can convert to something stateful at the API layer.)
Change-Id: I54665ec066a64eb0e48a06e2fcd0d2681a42df7f
Reviewed-on: https://boringssl-review.googlesource.com/7453
Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c
index b29d56c..2fc9094 100644
--- a/ssl/d1_clnt.c
+++ b/ssl/d1_clnt.c
@@ -447,12 +447,11 @@
break;
case SSL3_ST_CW_FLUSH:
- ssl->rwstate = SSL_WRITING;
if (BIO_flush(ssl->wbio) <= 0) {
+ ssl->rwstate = SSL_WRITING;
ret = -1;
goto end;
}
- ssl->rwstate = SSL_NOTHING;
ssl->state = ssl->s3->tmp.next_state;
break;
