Subject: Re: Possible bug relating to malloc()/realloc(), popen(), and read()
To: Sami Kantoluoto <sami.kantoluoto@embedtronics.fi>
From: der Mouse <mouse@Rodents.Montreal.QC.CA>
List: port-i386
Date: 12/02/2004 10:23:57
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <unistd.h>
>> #include <fcntl.h>
>> #include <err.h>
>> 
>> int
>> main(int argc, char *argv[])
>> {
>>   FILE   *cmd;
>>   int     cmd_fd;
>>   char   *output;
>>   char   *bufp;
>>   ssize_t count;
>>   size_t  bytes_read = 0;
>>   size_t  block_size = 1028;
>> 
>>   output = NULL;
>>   if ((cmd = popen(argv[1], "r")) == NULL)  return(-1);
>>   cmd_fd = fileno(cmd);

If you're using popen() you really ought to be using stdio, not
read(2), to read from it.  But in this case I think that's not a
practical problem even if it is a philosophical problem.

>>   if ((output = malloc(block_size)) == NULL)
>>       {
>>       warn("Could not allocate memory");
>>       return(-1);
>>       }
>> 
>>   bufp = output;
>>   bufp[0] = 0;
>>   
>>   while ((count = read(cmd_fd, bufp, block_size)) > 0)
>>       {
>>       bytes_read += count; 
>>       printf("*** Read %i bytes ***\n", count); // debug xxxx
>>       if (count == block_size)
>>           {
>>           if ((bufp = realloc(output, bytes_read + block_size)) == NULL)
>>               {
>>               warn("Could not allocate memory");
>>               output[bytes_read] = 0;
>>               return(-1);
>>               }
>>           output = bufp;
>>           bufp += bytes_read;

Here is your actual problem.

You need to move this bufp += bytes_read outside the if (ie, swap this
line with the next line).  Otherwise, you are failing to advance your
buffer pointer unless the read reads a whole block_size bytes, so of
_course_ it will be reading over previously read data.

Also, since you always try to read block_size bytes, you need to
realloc not when count==block_size but rather whenever
bytes_read+block_size is greater than the amount of memory you
currently have (which you don't keep anywhere) - ie, you need to make
sure you have at least block_size bytes for the next read to read into.

>>           }
>> 
>>       printf("---------------------------------------------\n"); // debug xxxx
>>       printf("%s\n\n", output); // debug xxxx
>>       }

What do you have against using stdio?

Leaving aside the question of using stdio or not, I'd write that as
something like this:

 /* I consider NULL harmful;
    I also consider gratuitous embedded assignments harmful. */
 output = malloc(block_size);
 if (output == 0) { ...can't malloc... }
 bufsize = block_size;
 fill = 0;
 while (1)
  { if (fill+block_size > bufsize)
     { bufsize += block_size;
       output = realloc(output,bufsize);
       if (output == 0) { ...can't realloc... }
     }
    count = read(cmd_fd,output+fill,bufsize-fill);
    if (count < 0) { ...read error... }
    if (count == 0) break; /* EOF */
    fill += count;
  }

/~\ The ASCII				der Mouse
\ / Ribbon Campaign
 X  Against HTML	       mouse@rodents.montreal.qc.ca
/ \ Email!	     7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B