[PATCH] bugfix: a single SSL record can't contain >16KiB, therefore we must loop when writing a larger buffer

[David Woodhouse]

On Wed, 2017-11-29 at 00:49 -0800, Daniel Lenski wrote:
> @@ -913,7 +913,11 @@ int do_https_request(struct openconnect_info *vpninfo, const char *method,
>         if (vpninfo->dump_http_traffic)
>                 dump_buf(vpninfo, '>', buf->data);
>  
> -       result = vpninfo->ssl_write(vpninfo, buf->data, buf->pos);
> +       for (int i=result=0; i<=buf->pos; i+=16384) {

We don't typically use C99 declaration rules in OpenConnect. Put your
'int i' at the beginning of a code block please.

I confess that's just a Luddite habit I carried over from the Linux
kernel coding style, and there's no good reason for it (hey, I often
rant about the stupidity of Linux refusing to use the C99 standard
integer types). And in other projects I'm actually much happier using
21st century C.

So I'm not entirely averse to changing the rules... but we do care
about compatibility, so it wants to be a conscious decision and
*tested* on all the random platforms we support, not just snuck in as
part of another change without comment :)

There are also a bunch of missing spaces, and it should look more like
> +       for (i = result = 0; i <= buf->pos; i += 16384) {

There is a reason for that kind of pedantry. Largely it's about making
things easier to read by being consistent. But especially for variable
assignment, I often do find myself searching for things like
'foo_variable = ' — and if someone has assigned to that variable
without the spaces, an assignment site gets missed and that leads to
bugs.

But look... having made it easier to read, I can see much more clearly
that you're setting 'result' too... and that makes me see that in fact,
you're just breaking out of the loop in the 'result < 0' case in order
to process that failure... when the failure handling could be *in* the
loop. So it could be a bit simpler if it were...


	for (i = 0; i <= buf->pos; i += 16384) {
		result = vpninfo->ssl_write(vpninfo, buf->data + i, MIN(buf->pos - i, 16384) );
		if (result < 0) {
			if (rq_retry) {
				/* Retry if we failed to send the request on
				   an already-open connection */
				openconnect_close_https(vpninfo, 0);
				goto retry;
			}
			/* We'll already have complained about whatever offended us */
			goto out;
		}
	}

(Grr, Evolution is screwing that up; maybe it'll actually send it OK)

Finally... is that 'i <= buf->pos' loop condition correct? That'll go
through the loop one last time when i == buf->pos and will attempt to
send zero bytes of data, won't it?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 4938 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/openconnect-devel/attachments/20171129/d5eccbcc/attachment.bin>