bug in proton-c sasl (was Re: Qpid C++ 1.36 issues an invalid AMQP 1.0 frame after successful DIGEST-MD5 authentication)

[Gordon Sim <gs...@redhat.com>]

On 09/01/17 22:43, Gordon Sim wrote:
> I have a 'fix' for this - i.e. a change that appears to prevent it
> occurring. If the broker waits after sending the sasl outcome for the
> client to send a protocol header, then I see no failures even running in
> a loop for an hour.
>
> I wonder whether the proton client's sasl layer is somehow getting
> (intermittently) upset if the last unencrypted frame and the first
> encrypted frame arrive together...

After some digging into the sasl implementation in proton-c, I *think* 
it is indeed a bug there.

In pn_input_read_sasl, when a successful outcome frame is read, it will 
set the desired_state to SASL_RECVED_OUTCOME_SUCCEED. This means that 
pni_sasl_is_final_input_state() will return true. However from what I 
can tell, the last_state, which is what is checked by 
pni_sasl_is_final_output_state(), is only set to this same value when 
pni_post_sasl_frame() is called, and the client will never need to call 
that after receiving an outcome (as the sasl exchange is then over).

So if we get two calls to pn_input_read_sasl(), one to read the outcome, 
the next to read the first encrypted data, *without* a 
pn_output_write_sasl() between them, pni_sasl_is_final_input_state() 
will return true but pni_sasl_is_final_output_state() will return false 
on the second read, which results in the second recv call passing it to 
the passthru layer even though it may be encrypted.

The attached patch seems to eliminate the problem. I don't really 
understand why the bit it removes was in there though so not sure if 
this is the 'right' fix.