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

[Neal H. Walfield]

Hi Daiki,

Thank you for this patch.  It looks quite good.  There are some minor
issues.  I can correct them if you don't have time, but I'd appreciate
it if you could just confirm my understanding.

Thanks,

:) Neal

At Thu, 28 May 2015 18:03:44 +0900,
Daiki Ueno wrote:
> +if test "$pinentry_emacs" = "maybe"; then
> +  AC_MSG_CHECKING([if Unix domain socket is supported])
> +  AC_TRY_COMPILE([
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +],
> +	         [int s = socket (AF_UNIX, SOCK_STREAM, 0);],
> +		 [_unixsock_works=yes],
> +		 [_unixsock_works=no])
> +  AC_MSG_RESULT($_unixsock_works)
> +  if test "$_unixsock_works" = "yes"; then
> +    pinentry_emacs=yes
> +  else
> +    pinentry_emacs=no
> +  fi
> +fi

I think we should check for != "no" here.  If pinentry_emacs is yes,
but we don't support unix domain sockets, then it would be better to
error out here than when compiling the code.  Do you agree?

> --- /dev/null
> +++ b/pinentry/pinentry-emacs.c

> +#include <assert.h>

As far as I can tell, this include is gratuitious.

> +  if (strlen (tmpdir) + strlen (socket_name) + 10 + 2
> +      >= sizeof (emacs_server_address.sun_path))
> +    {
> +      fprintf (stderr, "socket name is too long\n");
> +      free (tmpdir_storage);
> +      return 0;
> +    }
> +
> +  sprintf (emacs_server_address.sun_path, "%s/emacs%lu/%s",
> +	   tmpdir, (unsigned long) uid, socket_name);

I think the check above is wrong.  It should be something like:

  if (strlen (tmpdir) + strlen("/emacs") + 10 + strlen("/")
      + strlen (socket_name) + 1
      >= sizeof (emacs_server_address.sun_path))

Or, less conservatively, we could replace 10 with the actual length of
the uid as a base-10 string.

Perhaps the best approach is to use something like asprintf.  (I say
like, because we can't use asprintf: it is not portable.  But, recent
version of libgpg-error provide a replacement.)

> +static int
> +connect_to_server (void)
> ...
> +  if (connect (s, (struct sockaddr *) &emacs_server_address,
> +	       strlen (emacs_server_address.sun_path) + 2) < 0)

Why don't you just use 'sizeof (emacs_server_address)' here?  I really
don't like the magic number 2 here.

> +static int
> +read_from_emacs (int s, int timeout, char *result, size_t *lengthp)
> +{
> ...
> +      /* Loop over all NL-terminated messages.  */
> +      for (end_p = p = string; end_p != NULL && *end_p != '\0'; p = end_p)
> +	{
> +	  char *str;
> +	  size_t str_length;
> +	  end_p = strchr (p, '\n');
> +	  if (end_p != NULL)
> +	    *end_p++ = '\0';
> +	  if (strprefix ("-print ", p))
> +	    {
> +	      /* -print STRING: Print STRING on the terminal. */
> +	      str = p + strlen ("-print ");
> +	      unquote_argument (str, &str_length);
> +	      if (capacity < !!needlf + str_length)
> +		return 0;
> +
> +	      if (needlf)
> +		{
> +		  *out_p++ = '\n';
> +		  capacity--;
> +		}
> +
> +	      memcpy (out_p, str, str_length);
> +	      out_p += str_length;
> +	      capacity -= str_length;
> +	      needlf = str[0] == '\0' ? needlf : str[strlen (str) - 1] != '\n';
> +	    }
> +	  else if (strprefix ("-print-nonl ", p))
> +            {
> +              /* -print-nonl STRING: Print STRING on the terminal.
> +                 Used to continue a preceding -print command.  */
> +	      str = p + strlen ("-print-nonl ");
> +              unquote_argument (str, &str_length);
> +	      if (capacity < str_length)
> +		return 0;
> +	      memcpy (out_p, str, str_length);
> +	      out_p += str_length;
> +	      capacity -= str_length;
> +              needlf = str[0] == '\0' ? needlf : str[strlen (str) - 1] != '\n';
> +            }
> +          else if (strprefix ("-error ", p))
> +            {
> +              /* -error DESCRIPTION: Signal an error on the terminal. */
> +	      got_error = 1;
> +	      break;
> +            }
> +	}
> +    }

Does it make sense to emit a warning if there is a line with an
unrecognized prefix?  As far as I can see, lines with unknown commands
are currently ignored.

> +char *
> +build_command (const char *name, const char *value)
> +{
...
> +  command_length = strlen ("(" CALLBACK_NAME " \"\" )")
> +    + strlen (name) + quoted_value_length;
> +
> +  command = malloc (command_length + 1);
> +  if (!command)
> +    {
> +      perror ("malloc");
> +      return NULL;
> +    }
> +
> +  sprintf (command, "(" CALLBACK_NAME " \"%s\" %s)",
> +	   name, quoted_value);
> +  if (quoted_value != nil)
> +    free (quoted_value);

Here's another instance where I'd strongly prefer something like
asprintf.

> +int
> +set_value (pinentry_t pe, const char *name, const char *value)
> +{
> +  char buffer[16], *quoted_command, *local_value = value;
> +  size_t length;
> +  int s, success;
> +
> +  if (value)
> +    {
> +      local_value = pinentry_utf8_to_local (pe->lc_ctype, value);
> +      if (!local_value)
> +	return 0;
> +    }
> +
> +  quoted_command = build_command (name, local_value);
> +  if (local_value != value)
> +    free (local_value);

I think this comparison is useless.  If value is not NULL, then we
call pinentry_utf8_to_local, which always allocates a new buffer.

> +  if (!quoted_command)
> +    return 0;
> +
> +  s = connect_to_server ();
> +  if (s < 0)
> +    {
> +      free (quoted_command);
> +      return 0;
> +    }
> +
> +  length = sizeof (buffer);
> +  success = send_to_emacs (s, "-eval ")
> +    && send_to_emacs (s, quoted_command)
> +    && send_to_emacs (s, "\n")
> +    && read_from_emacs (s, pe->timeout, buffer, &length)
> +    && length == 1 && *buffer == 't';
> +
> +  free (quoted_command);
> +  close (s);
> +
> +  return success;
> +}
> +
> +int
> +do_password (pinentry_t pe)
> +{
> +  char buffer[16], *quoted_command;
> +  size_t length;
> +  int s, success;
> +
> +  set_value (pe, "SETTITLE", pe->title);
> +  set_value (pe, "SETDESC", pe->description);
> +  set_value (pe, "SETERROR", pe->error);
> +  set_value (pe, "SETPROMPT", pe->prompt);

Does emacs not support setting custom button labels?

If pe->prompt is NULL, then you should fallback to pe->default_prompt.

If you have time, it would be nice to add support for the external
password manager checkbox.  In the very least, please note the lack of
this support with an XXX comment in the source code.

> +	  if (pe->repeat_passphrase)
> +	    pe->repeat_okay = 1;

Why are you setting this if the user didn't actually enter the
password twice?

> +	  /* FIXME: Should we return the length of PASSWORD_UTF8, as
> +	     described in pinentry.h?  */
> +	  return 1;

I recently changed the API so that that is no longer necessary.  I see
I forgot to update the comment for pinentry_cmd_handler_t.  I'll do
that.


> +int
> +do_confirm (pinentry_t pe)
> +{
> +  char buffer[16], *quoted_command;
> +  size_t length;
> +  int s, success;
> +
> +  set_value (pe, "SETTITLE", pe->title);
> +  set_value (pe, "SETDESC", pe->description);
> +  set_value (pe, "SETERROR", pe->error);
> +  set_value (pe, "SETPROMPT", pe->prompt);

Again, please fallback to pe->default_prompt.

Any reason, there is no support for custom button labels?  Also, what
about support for the other button?  If this is not yet possible,
please note it with an XXX comment.



> diff --git a/pinentry/pinentry.c b/pinentry/pinentry.c
> index 9a6a090..0187309 100644
> --- a/pinentry/pinentry.c
> +++ b/pinentry/pinentry.c
> @@ -173,7 +173,7 @@ pinentry_assuan_reset_handler (ASSUAN_CONTEXT ctx)
>  
>  static int lc_ctype_unknown_warning = 0;
>  
> -#if defined FALLBACK_CURSES || defined PINENTRY_CURSES || defined PINENTRY_GTK
> +#if defined FALLBACK_CURSES || defined INSIDE_EMACS || defined PINENTRY_CURSES || defined PINENTRY_GTK
>  char *
>  pinentry_utf8_to_local (const char *lc_ctype, const char *text)
>  {

I think we should check if PINENTRY_EMACS is defined, not INSIDE_EMACS
here.