Re: [Bug-wget] [PATCH v4] (resend) Add --use-askpass=COMMAND support

[Liam R. Howlett]

* Liam R. Howlett <address@hidden> [160906 15:35]:
> * Tim R?hsen <address@hidden> [160903 15:44]:
> > Hi Liam,
> > 
> > thanks, we received the the FSF copyright assignment for Wget.
> > 
> > Can you give me an example of an external program to use with --use-askpass 
> > (maybe a 'standard' one available on common Linux distros) ?
> 
> The most common utility to use for the askpass interface is
> /usr/lib/ssh/x11-ssh-askpass
> This is included in ssh-askpass in debian.
> 
> I have also tested with ksshaskpass along with just small c binaries to
> dump long or short strings.
> 
> 
> > 
> > I still have trouble with:
> > +  /* Set the end byte to \0, and decrement bytes */
> > +  tmp[bytes--] = '\0';
> > +
> > +  /* Remove a possible new line */
> > +  while (bytes >= 0 &&
> > +        (tmp[bytes] == '\0' || tmp[bytes] == '\n' || tmp[bytes] == '\r'))
> > +    tmp[bytes--] = '\0';
> > +
> > +  *answer = xmemdup (tmp, bytes + 2);
> > 
> > You introduce a buffer overflow by one byte here.
> 
> 
> Ah, yes.  I've tested over 1024 and 1 character of '\n' and it works,
> but I'm underflowing bytes then overflowing it back to the correct
> value.

Small correction to my statement, bytes is ssize_t, so it goes negative
but doesn't underflow or overflow - it goes negative and then back
positive.  I still like your alternative below.

Thanks,
Liam

> 
> > 
> > You could write instead e.g.:
> > while (bytes > 0 &&
> >     (tmp[bytes - 1] == '\0' || tmp[bytes - 1] == '\n' || tmp[bytes - 1] == 
> > '\r'))
> >   bytes--;
> > 
> > *answer = xmemdup0(tmp, bytes);
> > 
> > (needs to add xmemdup0 gnulib module to bootstrap.conf)
> > 
> > And if you do that, you can also:
> >   bytes = read (com[0], tmp, sizeof (tmp));
> > instead of
> >   bytes = read (com[0], tmp, sizeof (tmp) - 1);
> 
> I am happy to change my code like you suggest above for clarity if you
> are okay with adding xmemdup0 to bootstrap.conf.  I will resend v5 once
> I retest with this update.
> 
> > 
> > The patch looks fine otherwise ! Just have to test it with a program - do 
> > you 
> > have something in mind ?
> 
> There are quite a few, debian & ubuntu set up an alternatives link for
> ssh-askpass which can point to ksshaskpass, ssh-askpass-gnome,
> ssh-askpass-fullscreen, and ssh-askpass.  ssh-askpass homepage is
> http://www.jmknoble.net/software/x11-ssh-askpass/ according to apt-cache
> show ssh-askpass, but the link seems dead.  Here is the debian.org
> stable link: https://packages.debian.org/source/jessie/ssh-askpass
> Since your mail client appears to be kmail, you could try ksshaskpass:
> https://quickgit.kde.org/?p=ksshaskpass.git
> 
> 
> Thanks,
> Liam
> 
> > 
> > Regards, Tim
> > 
> > 
> > On Donnerstag, 1. September 2016 11:22:32 CEST Liam R. Howlett wrote:
> > > This adds the --use-askpass option which is disabled by default.
> > > 
> > > --use-askpass=COMMAND will request the username and password for a given
> > > URL by executing the external program COMMAND.  If COMMAND is left
> > > blank, then the external program in the environment variable
> > > WGET_ASKPASS will be used.  If WGET_ASKPASS is not set then the
> > > environment variable SSH_ASKPASS is used.  If there is no value set, an
> > > error is returned.  If an error occurs requesting the username or
> > > password, wget will exit.
> > > 
> > > 
> > > I am resending this patch because the FSF Contribution Agreement has been
> > > signed by our legal team.
> > > 
> > > Liam R. Howlett (1):
> > >   Add --use-askpass=COMMAND support
> > > 
> > >  bootstrap.conf |   1 +
> > >  doc/wget.texi  |  17 ++++++---
> > >  src/init.c     |  44 +++++++++++++++++++++++
> > >  src/main.c     | 112
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/options.h  
> > > | 
> > >  1 +
> > >  src/url.c      |   6 ++++
> > >  src/url.h      |   1 +
> > >  7 files changed, 178 insertions(+), 4 deletions(-)
> > 
> 
>