assuan_read_from_server() and return value

[Ben Kibbey]

On Mon, Dec 03, 2007 at 03:56:53PM +0100, Marcus Brinkmann wrote:
> Hi Ben,
> 
> there seem to be separate issues here:
> 
> 1) It seems to me that the case you describe can only happen if you
> have no inquire callback set, right?  In this case, the error code is
> going to be overwritten by assuan_transact anyway.

After the inquire callback is finished, assuan_transact() sends END to
terminate the inquire then calls _assuan_read_from_server() again. On
the server end, an error may have occured while processing the inquired
data which may return an error code which is not guaranteed to be an
ASSUAN_ error code. Without the patch, _assuan_read_from_server()
returns the value returned from _assuan_read_line() (zero). I use
GPG_ERR codes on the server. Pinentry uses ASSUAN_ errors and there is
no way to tell if the error is an GPG_ERR_ or an ASSUAN_ error. So I
guess pinentry has to migrate to use GPG_ERR_ codes, which may break
clients, or I have to wait until libassuan removes ASSUAN_ codes
completely forcing libassuan users to use GPG_ERR_ codes, which seems
unlikely to happen any time soon?

> Frankly, I do not understand why there is a dummy read in that place
> anyway.  Comparing this with the code in assuan-buffer.c, where there

Maybe to flush the input?

> is no dummy read after sending the END command (this is the code path
> taken if there is an inquire callback), the dummy read does not seem
> to be necessary, in fact, in my opinion it could be downright harmful.
> OTOH, as there is an error code returned by assuan_transact, one could
> argue that the connection is in a messed up state anyway at that point
> due to the missing inquire callback.  The code may be designed to
> salvage from the connection whatever is possible.  But, this fails if
> for example after sending END and doing a dummy read the server
> command is still not yet finished.
> 
> It seems to me the right way to fix this is to send a CANCEL instead
> or following the END.
> 
> 2) However, the function _assuan_read_from_server is also used in two
> other places in libassuan, where an error could also be returned due
> to a missing handshake.  In this case, the return code could be
> useful.  I think it is reasonable to assume that it is a gpg error or
> assuan error code at that point, because the low-level handshake
> protocol is defined by assuan and we control the sender and receiver
> side for this.  In fact, we can always just return whatever ERR number
> is provided by the server: assuan_error_t is defined in that way (some
> values are reserved for the assuan protocol, the rest is free.  We
> recently introduced support for gpg-error numbers, which makes things
> a bit more delicate, but I am willing to ignore this complication for
> the sake of discussion :).

Heh. This is the main problem I've having. The ERR returned from the
server isn't known to be ASSUAN_ or GPG_ERR_. If all servers used
GPG_ERR, it'd fix things for me. Or maybe just all clients that use
pinentry after patching it.

> So, I think I would still like to apply your patch, although it should
> not make any difference in the scenario you describe.  Plus, I would
> like to replace the dummy read and possibly (if it works) the sending
> of END by a sending of CANCEL.

If CANCEL is used, wouldn't the INQUIRE on the server end not succeed
(OK response) if all went well?

-- 
Benjamin J. Kibbey bjk at luxsci.net/jabber/freenode
3019 F5FC AA33 5BC7 BE9F 09D2 393E DBD2 40D5 FA7E