[scponly] Re: scponly bug

[Zdenek Hladik]

On 17 Sep 2002, at 11:26, joe wrote:

> 
> karl,
> 
> i'm not familiar with split, is it a source code analyzer?

He probably mean some kind of c code checker - lint.

> 
> regarding realloc, you're right that the free before the exit is probably
> not neccesary, however, to explain the origin of this code, keep reading:
> 
> the original code in helper.c:
>                 if (NULL == (temp = realloc (outbuf, newlen)))
>                 {
>                         perror("realloc");
>                         if (outbuf)
>                                 free(outbuf);
>                         exit(-1);
>                 }
>                 outbuf=temp;
>                 temp=NULL;
> 
> the openbsd realloc() manpage:
>      When using realloc() one must be careful to avoid the following
> idiom:
> 
>            if ((p = realloc(p, nsize)) == NULL)
>                    return NULL;
> 
>      In most cases, this will result in a leak of memory.  As stated
> earlier,
>      a return value of NULL indicates that the old object still remains
> allo-
>      cated.  Better code looks like this:
> 
>            if ((p2 = realloc(p, nsize)) == NULL) {
>                    if (p)
>                            free(p);
>                    p = NULL;
>                    return NULL;
>            }
>            p = p2;
> 
> 
> granted, the exit instead of the free makes this all a moot point.

OK but in this case you use relatively very small chunks of memory so 
case of unsuccessful realloc does apear very rarely. 

> 
> also, the exit value of -1 is something i agree should be changed.  it
> will be included in the next rev.
> 
> joe
> 
> 
> ----
> 
> PGP KEY: http://www.sublimation.org/contact.html
> PGP Key fingerprint = EC4B 0DA5 B4F6 BDDD 9176 55D6 3A6A 7D63 158F 22D2
> 
> 
> On 17 Sep 2002, Karl DeBisschop wrote:
> 
> > On Tue, 2002-09-17 at 03:58, joe wrote:
> >
> > > > > On Mon, 16 Sep 2002, Zdenek Hladik wrote:
> > > > >
> > > > > > But more serious problem i got. At first I believed that i made wrong
> > > > > > chroot jail, but after adding some debug messages to scponly.c i
> > > > > > found that scponlyc crashes inside
> > > > > >
> > > > > >        flatten_vector()
> > > > > >
> > > > > > on processing of scp -r -p -d "somefile" command from winscp. with
> > > > > > exit signal 11 (memory violation).
> >
> > splint flags the following code as a use of already freed memory:
> >
> > 		if (NULL == (temp = realloc (outbuf, newlen)))
> > 		{
> > 			perror("realloc");
> > 			if (outbuf)
> > 				free(outbuf);
> > 			exit(-1);
> > 		}
> >
> > I think it is right. I think of it like this: realloc takes an existing
> > memory segment, and hands you a new segmant of the requested size,
> > preserving the contents of the old segment if it fits within the length
> > of the returned memory space (which may or may not start at the same
> > point as the old pointer). So outbuf is actually freed by realloc.
> >
> > Now it could be that I'm not uderstanding it fully, but if the logic
> > above (and the splint output) are correct, then
> >
> > 		if (NULL == (temp = realloc (outbuf, newlen)))
> > 		{
> > 			perror("realloc");
> > 			exit(-1);
> > 		}
> >
> > is the correct code.
> >
> > Even if there's argument about whether or not it's correct, omitting the
> > free() does clear up the splint error, and since the next call is to
> > exit(), there would be no real consequence to leaving outbuf unfreed.

I dont see what is wrong. I think Joe's usage is correct. So there in 
not vasted memory because on realloc failure outbuf pointed buffer is 
not dealocated 


But I thing crash of scponly is caused by buffer overflow on first 
pass. (bug on line nr.58 inside helper.c.
all next passes inside while loop writes one byte more than 
reallocated buffer size. Efect of this may be random...


> >
> > --
> > Karl DeBisschop <kdebisschop at alert.infoplease.com>
> >
> >
>