[PATCH] cinstall - "setup grabbing memory by the bucketfull" problem found

[Pavel Tsekov <ptsekov at syntrex dot com>]

Christopher Faylor wrote:
> 
> On Tue, Nov 06, 2001 at 03:15:22PM +0100, Pavel Tsekov wrote:
> >Attached is a patch which initializes all global state variables used
> >in cinstall with zero values.  The patch is a result from a discussion
> >in the cygwin maling list about a strange behaviour of setup.exe
> >resulting in heavy memory usage and process crash/freeze.  The
> >following link gives additional information about this behaviour:
> >http://sources.redhat.com/ml/cygwin/2001-11/msg00304.html
> 
> If you have to initialize globals to zero something is drastically wrong
> somewhere.
> 
> I would suggest finding out why this patch is necessary.  It sounds like
> a shotgun approach to the problem to me.

Indeed it was like a 'shotgun approach' and I've apologized for loosing
people's time :(.

Ok, now back to the problem... Last night I've found what's going on and
here I'll provide a simple testcase for anyone to test for himself and 
also a very simple patch that fixes the problem. My system at home is 
Win2k professional, but actually this shouldnt matter too much.

So, to make setup.exe start eating memory like hell, locking the whole
system, while retrieving setup.ini, one can unplug the network cable ;)
Another choice is to give a unexistent hostname using the "oher site"
dialog box.

The purpose for this behaviour is that the constructor of the NetIO_FTP
class tries to read/write using the SimpleSocket object for the FTP
command channel, without first checking to see if the command channel
socket object is ok(). The actual lock happens on the first ftp_line()
call. What happens is that if the command socket is invalid, each read
on it will not fill the internal buffer thus returnin a valid pointer 
to ftp_line but this pointer contains garbage. This in turn makes
ftp_line()
loop forever.

Ok now about the patch - I'm just attaching it for review if someone
decides
to apply it I'll provide a Changelog. What I think is that this is not
the best
solution to the problem (though it works just fine) - a better one maybe
will be
some more checking in the SimpleSocket class. However I understand that
the name
Simple implies simplicity :) and as such the designers of this class
decided to
leave the error checking for the caller, not the object itself. So my
patch
complies with this and just check  if the object is valid after its
creation.

Another thing I want to say is that as a side effect of investigating
this 
bug, I've found another problem in the NetIO_FTP class, maybe its not so
important though: The Microsoft FTP Server 5.0 (IIS which comes with
Win2k)
returns 125 on RETR instead of 150 as expected from setup.exe so you
cant
retrieve files from that server. I dont know if previous or more recent 
microsoft ftp servers behave like this. Changing line 130 of nio-ftp.cc
to
if (code != 150 && code != 125) fixes the problem.

I hope this helps.
--- nio-ftp.cc.ORIG	Wed Nov  7 10:27:54 2001
+++ nio-ftp.cc	Wed Nov  7 10:36:47 2001
@@ -70,6 +70,12 @@ NetIO_FTP::NetIO_FTP (char *Purl, BOOL a
   if (cmd == 0)
     {
       SimpleSocket *c = new SimpleSocket (host, port);
+      if (!c || !c->ok())
+        {
+          delete c;
+          return;
+        }
+
       int done = 0;
       code = ftp_line (c);
 
@@ -124,6 +130,12 @@ auth_retry:
   char tmp[20];
   sprintf (tmp, "%d.%d.%d.%d", i1, i2, i3, i4);
   s = new SimpleSocket (tmp, p1*256 + p2);
+  if (!s || !s->ok())
+    {
+      delete s;
+      s = 0;
+      return;
+    }
 
   cmd->printf ("RETR %s\r\n", path);
   code = ftp_line (cmd);