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