Browse code

tls: fix wrong wbio usage

The openssl library sometimes (after write operations) sets a
buffering bio "over" the wbio that we set. In this case one can no
longer rely on the wbio returned by SSL_get_wbio() (it might be the
buffering bio).
Now the BIOs are set at connection initialization time (and not on
their first use) and are stored inside the tls_extra_data
structure attached to the tcp connection. This way we are sure we
are always controlling our wbio and not something that openssl
might have stacked on top of it.

Andrei Pelinescu-Onciul authored on 04/06/2010 16:30:48
Showing 3 changed files
... ...
@@ -115,7 +115,7 @@ int tls_BIO_mbuf_set(BIO* b, struct tls_mbuf* rd, struct tls_mbuf* wr)
115 115
 {
116 116
 	struct tls_bio_mbuf_data* d;
117 117
 	
118
-	TLS_BIO_DBG("tls_BIO_muf_set called (%p, %p)\n", rd, wr);
118
+	TLS_BIO_DBG("tls_BIO_mbuf_set called (%p => %p, %p)\n", b, rd, wr);
119 119
 	if (unlikely(b->ptr == 0)){
120 120
 		BUG("null BIO ptr\n");
121 121
 		return 0;
... ...
@@ -195,7 +195,8 @@ static int tls_bio_mbuf_read(BIO* b, char* dst, int dst_len)
195 195
 				   as a shortcut when no data is available =>
196 196
 				   simulate EAGIAN/WANT_READ */
197 197
 				TLS_BIO_DBG("read (%p, %p, %d) called with null read buffer"
198
-						" => simulating EAGAIN/WANT_READ\n", b, dst, dst_len);
198
+						"(%p->%p) => simulating EAGAIN/WANT_READ\n",
199
+						b, dst, dst_len, d, d->rd);
199 200
 				BIO_set_retry_read(b);
200 201
 			}
201 202
 			return -1;
... ...
@@ -126,11 +126,16 @@ static int tls_complete_init(struct tcp_connection* c)
126 126
 	}
127 127
 	memset(data, '\0', sizeof(struct tls_extra_data));
128 128
 	data->ssl = SSL_new(dom->ctx[process_no]);
129
+	data->rwbio = tls_BIO_new_mbuf(0, 0);
129 130
 	data->cfg = cfg;
130 131
 	data->state = state;
131 132
 
132
-	if (data->ssl == 0) {
133
-		TLS_ERR("Failed to create SSL structure:");
133
+	if (unlikely(data->ssl == 0 || data->rwbio == 0)) {
134
+		TLS_ERR("Failed to create SSL or BIO structure:");
135
+		if (data->ssl)
136
+			SSL_free(data->ssl);
137
+		if (data->rwbio)
138
+			BIO_free(data->rwbio);
134 139
 		goto error;
135 140
 	}
136 141
 #ifdef TLS_KSSL_WORKARROUND
... ...
@@ -140,6 +145,7 @@ static int tls_complete_init(struct tcp_connection* c)
140 140
 		data->ssl->kssl_ctx=0;
141 141
 	}
142 142
 #endif
143
+	SSL_set_bio(data->ssl, data->rwbio, data->rwbio);
143 144
 	c->extra_data = data;
144 145
 	return 0;
145 146
 
... ...
@@ -182,25 +188,9 @@ static int tls_set_mbufs(struct tcp_connection *c,
182 182
 							struct tls_mbuf* rd,
183 183
 							struct tls_mbuf* wr)
184 184
 {
185
-	SSL *ssl;
186 185
 	BIO *rwbio;
187 186
 	
188
-	/* if (unlikely(tls_fix_connection(c) < 0))
189
-		return -1;
190
-	*/
191
-	
192
-	ssl = ((struct tls_extra_data*)c->extra_data)->ssl;
193
-	if (unlikely(((rwbio=SSL_get_rbio(ssl))==0) ||
194
-					((rwbio=SSL_get_wbio(ssl))==0))) {
195
-		rwbio = tls_BIO_new_mbuf(rd, wr);
196
-		if (unlikely(rwbio == 0)) {
197
-			ERR("new mbuf BIO creation failure\n");
198
-			return -1;
199
-		}
200
-		/* use the same bio for both read & write */
201
-		SSL_set_bio(ssl, rwbio, rwbio);
202
-		return 0;
203
-	}
187
+	rwbio = ((struct tls_extra_data*)c->extra_data)->rwbio;
204 188
 	if (unlikely(tls_BIO_mbuf_set(rwbio, rd, wr)<=0)) {
205 189
 		/* it should be always 1 */
206 190
 		ERR("failed to set mbufs");
... ...
@@ -875,7 +865,7 @@ redo_read:
875 875
 	*/
876 876
 	if (unlikely(tls_c->enc_rd_buf)) {
877 877
 		/* use queued data */
878
-		/* safe to use without locks, because only read changes it and 
878
+		/* safe to use without locks, because only read changes it and
879 879
 		   there can't be parallel reads on the same connection */
880 880
 		enc_rd_buf = tls_c->enc_rd_buf;
881 881
 		tls_c->enc_rd_buf = 0;
... ...
@@ -980,8 +970,8 @@ ssl_read_skipped:
980 980
 				goto error_send;
981 981
 			}
982 982
 		}
983
-		/* quickly catch bugs: segfault if accessed and not set */
984
-		tls_set_mbufs(c, 0, 0);
983
+	/* quickly catch bugs: segfault if accessed and not set */
984
+	tls_set_mbufs(c, 0, 0);
985 985
 	lock_release(&c->write_lock);
986 986
 	switch(ssl_error) {
987 987
 		case SSL_ERROR_NONE:
... ...
@@ -1027,11 +1017,12 @@ ssl_read_skipped:
1027 1027
 		if (unlikely(n < 0))
1028 1028
 			/* here n should always be >= 0 */
1029 1029
 			BUG("unexpected value (n = %d)\n", n);
1030
-		else if (unlikely(n < bytes_free))
1031
-			BUG("read buffer not exhausted (rbio still has %d bytes,"
1032
-					"last SSL_read %d / %d)\n",
1033
-					rd.used - rd.pos, n, bytes_free);
1034
-		else if (n == bytes_free) {
1030
+		else {
1031
+			if (unlikely(n < bytes_free))
1032
+				BUG("read buffer not exhausted (rbio still has %d bytes,"
1033
+						"last SSL_read %d / %d)\n",
1034
+						rd.used - rd.pos, n, bytes_free);
1035
+			/* n <= bytes_free */
1035 1036
 			/*  queue read data if not fully consumed by SSL_read()
1036 1037
 			 * (very unlikely situation)
1037 1038
 			 */
... ...
@@ -55,6 +55,9 @@ struct tls_rd_buf {
55 55
 struct tls_extra_data {
56 56
 	tls_domains_cfg_t* cfg; /* Configuration used for this connection */
57 57
 	SSL* ssl;               /* SSL context used for the connection */
58
+	BIO* rwbio;             /* bio used for read/write
59
+							   (openssl code might add buffering BIOs so
60
+							    it's better to remember our original BIO) */
58 61
 	tls_ct_q* ct_wq;
59 62
 	struct tls_rd_buf* enc_rd_buf;
60 63
 	unsigned int flags;