Re: [PATCH] Improve application data throughput

[Simon Josefsson]

Enrico Scholz <address@hidden> writes:

> Simon Josefsson <address@hidden> writes:
>
>>> There was added an overflow check too which assumes 1MiB as maximum
>>> size for a GSASL message.  This value is a blind guess; please adjust
>>> when specifications require a higher value.
>>
>> I don't understand this part -- can you explain why it is needed?  
>
> Both
>
> |-              sockpos++;
> |+              sockpos += len;
>
> operations can wrap and they (resp. the called recv functions) can cause
> buffer overflows.  We could check for this wrap, or (as I did) limit the
> maximum buffer size to prevent DOS attacks which allocate huge amounts
> of memory.
>
> Such limits should be chosen that they do not violate specifications
> resp. real-world implementations.  I chose 1 MiB based upon my
> observations.

Thanks for explaining, I now agree.

>>> I see only one problem with the patch:  when remote site stops sending
>>> in the middle of a TLS record, the 'gsasl' application will hang in
>>> gnutls_record_recv() forever.
>>
>> When would that happen?  If the network connection goes down? 
>
> yes; e.g. somebody unplugs a cable
>
>
>> I thought there were some TCP timeouts that would kick in after some
>> time without any data.
>
> afaik, recv() does not have such a timeout (it does not send packets
> which require an ACK and won't get information about dropped connection
> hence).
>
>
> With old code, waiting would happen exclusively in select() which listens
> both for network and for application events.  When application sees that
> there did not came data for a certain time, it could close its input
> pipe, select() returns and 'gsasl' sees that application hung up and
> exits.
>
> With new code, application would have to send a signal and abort 'gsasl'
> process.
>
> It should be possible to put network and perhaps application sockets in
> non-blocking mode and restore the old behaviour.  But I did not tested
> it.

I think this is an acceptable risk, let's use your patch and see if
anyone experiences real-world problems.  If they do, we can try to debug
and fix it then, which could involve revert this patch but most likely
coming up with something else to handle the problem.

/Simon