[PATCH] Add inside-Emacs mode to GUI pinentry programs

[Daiki Ueno]

Thanks for the review, I'm attaching a new patch.

"Neal H. Walfield" <neal at walfield.org> writes:

>> +static int
>> +send_to_emacs (int s, const char *buffer)
>> +{
[...]
>> +      if (send_buffer_length == SEND_BUFFER_SIZE
>> +	  || (send_buffer_length > 0
>> +	      && send_buffer[send_buffer_length-1] == '\n'))
>
> Should we check for embedded newlines and not just those at the end?

For our use-cases of send_to_emacs, we can assume that a newline
character only appears at the end of buffer.

The comment of send_to_emacs (copied from emacsclient.c) says:

  /* Let's send the data to Emacs when either
     - the data ends in "\n", or
     - the buffer is full (but this shouldn't happen)
     Otherwise, we just accumulate it.  */

and a typical usage of the function:

  if (!send_to_emacs (emacs_socket, name)
      || !send_to_emacs (emacs_socket, " "))
    return 0;

  escaped = escape (value);
  if (!escaped)
    return 0;

  retval = send_to_emacs (emacs_socket, escaped)
    && send_to_emacs (emacs_socket, "\n");

Here, a newline character will never be passed to send_to_emacs, until
the last call of the function.  Perhaps the comment should state that
more clearly.

>> +read_from_emacs (int s, int timeout, char *buffer, size_t capacity,
>> +		 assuan_error_t *error)
>
> I think this function assumes that responses are line buffered.  That
> is, emacs will never send a single message consisting of "OK\nOK\n".
> Is that reasonable?

Yes, I think so.  A send_to_emacs/read_from_emacs pair basically
emulates assuan_transact, which expects at most one response code which
completes a transaction (i.e., OK/ERR/END).

>> +	      if (allocated && offset + data_length + 1 > allocated_capacity)
>> +		{
>> +		  allocated_capacity = allocated_capacity * 2 + 10;
>
> I think this is wrong.  allocated_capacity has to be at least as large
> as offset + data_length + 1.  If data_length is more than
> allocated_capacity + 10, then we overflow when we copy the data into
> buffer (this is easy when buffer is NULL).  How about this?
>
>   allocated_capacity = MAX (allocated_capacity * 2 + 10,
>   			    offset + data_length + 1);

Oops, thanks for pointing that out.  Fixed.

> Since pinentry_inside_emacs calls pinentry_emacs_init should we make
> sure pinentry_emacs_init is only called once (perhaps by checking
> whether emacs_socket is -1)?

Good point, but should we consider the case where pinentry_inside_emacs
is called multiple times?  Assuming not, I added an assertion in
pinentry_emacs_init to prevent the socket being initialized twice, and
renamed pinentry_inside_emacs to a clearer name denoting a side effect:
pinentry_enable_emacs_cmd_handler.

Also, I moved the call to the function right after pinentry_parse_opts,
aiming for the option dependent behavior, which I wrote in the reply to
Daniel:

> GnuPG not to interact with Emacs.  In that case, it might make sense to
> add an option to pinentry.conf, e.g., {no-,}allow-emacs-pinentry.
>
> I will try to add it in the new patch.

I postpone this for now, since it turned out that it requires a change
in GnuPG.

Regards,
-- 
Daiki Ueno
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-inside-Emacs-mode-to-GUI-pinentry-programs.patch
Type: text/x-patch
Size: 30311 bytes
Desc: not available
URL: </pipermail/attachments/20150609/b35e1aef/attachment-0001.bin>