CODEWITHSUNDEEP
carrayslinuxpopen
OpenSourceEnthusiast
OpenSourceEnthusiast

Reputation: 101

popen and output of system command

I have to figure out the available space in /mnt/ in my application. I wrote the following code. However, execute_cmd some times returns junk apart from the actual output. For ex: 4.5K(followed by junk). Where am I going wrong? Could some one review and let me know why execute_cmd returns a junk byte at the end? How do I improve the code? 

char *execute_cmd(char *cmd)
{
        FILE *fp;
        char path[100];
        int ii = 0;
        //char ii = 0;
        char *buffer = malloc(1024);
        char len = 0;

        /* Open the command for reading. */
        fp = popen(cmd, "r");
        if (fp == NULL) {
        printf("Failed to run command\n" );
        exit(1);
        }
        printf("Running command is: %s\n", cmd);

        memset(buffer, 0, sizeof(buffer));

        do {
        len = fread(path, 100, 1, fp); /* Is it okay to use fread? I do not know how many bytes to read as this function is a generic function which can be used for executing any command */
        strcat(buffer,path);
        printf("Number of bytes is: %d\n", len);
        } while (len != 0);

        len = strlen(buffer);
        printf("Buffer contents are: %s %d\n", buffer,len);

        /* close */
        pclose(fp);
}

void main()
{
        char *buffer = "df -h  | grep \"/mnt\" | awk '{ print $4}'"; /* FIXME */
        char len;
        char units;
        float number;
        char dummy = 0;
        char *avail_space;

        avail_space = execute_cmd(buffer);
        len = strlen(avail_space);
        units = avail_space[len - 1];

        printf("Available space is: %s %d %c end here\n", avail_space, len, units);
        number = strtof(avail_space, NULL);
        printf("Number is: %f\n", number);
}

Upvotes: 0

Views: 333

Answers (1)

rici
rici

Reputation: 241701

sizeof(buffer) is sizeof(char*), which is probably 8 (or maybe 4). So your memset only clears a little bit of buffer. But with your use of fread, it's not just buffer that needs to be cleared; it's the temporary path.

Uninitialized local variables like path are not zero-initialised. You could use memset(path, 0, sizeof(path)); to clear it -- here the sizeof works because path really is an array -- but simpler is to initialise it in the declaration: char path[100] = "";.

Since fread does not NUL-terminate what it reads, there might be arbitrary garbage following it, making the strcat Undefined Behaviour. In fact, the strcat is totally unnecessary and a waste of cycles. You know how much data you read (it's in len) so you know exactly where to read the next chunk and you can do so directly without a temporary buffer and without a copy.

For future reference, if you are planning on calling malloc and then using memset to clear the allocated region, you should instead use calloc. That's what it's there for.

Upvotes: 1

Related Questions