[scponly] Re: scponly bug

[joe]

karl,

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

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.

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.
>
> --
> Karl DeBisschop <kdebisschop at alert.infoplease.com>
>
>