Re: [bug-inetutils] inetutils 1.9.1-4: ftp "get" command occasionally dumps core,

[Mats Erik Andersson]

Dear Glenn,

onsdag den 11 september 2013 klockan 04:42 skrev Glenn Golden detta:
> --
> Mats Erik Andersson <address@hidden> [2013-09-10 13:14:04 +0200]:
> 
> >
> > On the good side is the fact that my patch series removes six segfaults!
> >
> 
> Maybe seven. :)  I just tried your git head, numerous times, and can't get
> "my" segfault to occur with it. Gee, little sad, like a long lost friend
> departed. :)
> 
> Seriously though... not sure if this is good news or not, since it sounded
> (from your prior ML message) that you were suspecting that this particular
> segfault was more likly due to issues other than those you had addressed.

I had no clear view, only new that the code in HEAD was broken.

> But perhaps I misunderstood ...? Please educate me on this, I may have
> misinterpreted whether you were or were not expecting your static->malloc()
> fixes to kill this problem.

It would have been best to identify the exact reason of failure,
but this added progress in valuable still. My recent patch set
identified essentially three weak spots in the 1.9.1 code, most
is in common with the legacy BSD code:

   ftp/main.c:  makeargv() and slurpstring() are acting on "line",
                "argbase", and "stringbase" with no knowledge about
                their sizes. cmdscanner() checks string length against
                MAXLINE, but this snippet was once introduced to
                avoid an obvious buffer overflow. cmdscanner() calls
                makeargv() with dynamic "line".

   ftp/domacro.c:  domacro() uses a fixed length on LINE2, whereas "line"
                   is still dynamic. A strcpy() could cause off-by-one
                   overflow (cmdscanner has "strlen>=MAXLINE) on line2.
                   Argument expansion inserts text with strcpy() into
                   the presumably very short "line".

   ftp/ruserpass.c  The size of "static char tokval[100]" is only half
                    of the needed MAXLINE. token() is wildly writing
                    into tokval.

> Btw, just for completeness, I verified that the segfault does still
> occur with the 1.9.1 version (binary from the distro) and also with
> 1.9.1 built from the sources against whatever versions of various
> libs are installed on this machine right now (since I had synched
> it to the repos very recently).  It does still occur with both.

Your observation could be in domacro(). Initially you type essentially

  line = strdup ("$myget");

Then your macro expansion needs

  strcpy(line + offset, "xmltoman-0.4-1.src.tar.gz");

The allocation of "line" happened to produce some surplus space
thanks to block requests, so the string fits, but does strcpy
overwrite allocation structures needed for recovery at _exit()?

> 
> So anyway... regardless of whether it was "expected" to be fixed or not... it
> evidently is, so thank you for your attention and diligence on this.  Pleasure
> to work with you on it.

Your input has been invaluable, and your initial observation was
intrumental in getting my attention on these details. Your own
contribution is very honorable indeed. Hopefully the maintainer
of inetutils in Arch Linux will give you credit for all your work.

Best regards,
  Mats E A