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

[Neal H. Walfield]

Hi Daiki,

It looks great.  Just a couple of small comments.  I think we almost
there.

Thanks!

:) Neal

At Mon, 08 Jun 2015 13:05:01 +0900,
Daiki Ueno wrote:
> +static int
> +send_to_emacs (int s, const char *buffer)
> +{
> +  size_t length;
> +  int retval = 1;
> +
> +  length = strlen (buffer);
> +  while (*buffer)
> +    {
> +      size_t part = MIN (length, SEND_BUFFER_SIZE - send_buffer_length);
> +      memcpy (&send_buffer[send_buffer_length], buffer, part);
> +      buffer += part;
> +      send_buffer_length += part;
> +
> +      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?
That is, something like this:

  memchr (send_buffer, '\n', send_buffer_length)

> +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?

> +	      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);

> +int
> +pinentry_inside_emacs (void)
> +{
> +  const char *envvar;
> +
> +  /* Check if INSIDE_EMACS envvar is set.  */
> +  envvar = getenv ("INSIDE_EMACS");
> +  if (!envvar || !*envvar)
> +    return 0;
> +
> +  /* FIXME: Additional checks for the value.  */
> +  return pinentry_emacs_init ();
> +}
> +
> +int
> +pinentry_emacs_init (void)
> +{
> +  char buffer[256];
> +  assuan_error_t error;
> +
> +  /* Check if we can connect to the Emacs server socket.  */
> +  if (!set_socket ("pinentry"))
> +    return 0;
> +
> +  /* Check if the server responds.  */
> +  read_from_emacs (emacs_socket, INITIAL_TIMEOUT, buffer, sizeof (buffer),
> +		   &error);
> +  return error == ASSUAN_No_Error;
> +}

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)?  Alternatively, if it has already been
called, then we should release emacs_socket.