I probably got an easy one for the C programmers out there!

I am trying to create a simple C function that will execute a system command in and write the process output to a string buffer out (which should be initialized as an array of strings of length n). The output needs to be formatted in the following way:

Each line written to stdout should be initialized as a string. Each of these strings has variable length. The output should be an array consisting of each string. There is no way to know how many strings will be written, so this array is also technically of variable length (but for my purposes, I just create a fixed-length array outside the function and pass its length as an argument, rather than going for an array that I would have to manually allocate memory for).

Here is what I have right now:

#define MAX_LINE_LENGTH 512                                    
int exec(const char* in, const char** out, const size_t n)
{
    char buffer[MAX_LINE_LENGTH];
    FILE *file;

    const char terminator = '\0';

    if ((file = popen(in, "r")) == NULL) {
        return 1;
    }
    
    for (char** head = out; (size_t)head < (size_t)out   n && fgets(buffer, MAX_LINE_LENGTH, file) != NULL; head  = strlen(buffer)) {
        *head = strcat(buffer, &terminator);
    }

    if (pclose(file)) {
        return 2;
    }

    return 0;
}

and I call it with

#define N 128 
int main(void)
{
    const char* buffer[N];
    const char cmd[] = "<some system command resulting in multi-line output>";
    const int code = exec(cmd, buffer, N);
    exit(code);
}

I believe the error the above code results in is a seg fault, but I'm not experienced enough to figure out why or how to fix.

I'm almost positive it is with my logic here:

for (char** head = out; (size_t)head < (size_t)out   n && fgets(buffer, MAX_LINE_LENGTH, file) != NULL; head  = strlen(buffer)) {
        *head = strcat(buffer, &terminator);
}

What I thought this does is:

1. Get a mutable reference to out (i.e. the head pointer)
2. Save the current stdout line to buffer (via fgets)
3. Append a null terminator to buffer (because I don't think fgets does this?)
4. Overwrite the data at head pointer with the value from step 3
5. Move head pointer strlen(buffer) bytes over (i.e. the number of chars in buffer)
6. Continue until fgets returns NULL or head pointer has been moved beyond the bounds of out array

Where am I wrong? Any help appreciated, thanks!

EDIT #1

According to Barmar's suggestions, I edited my code:

#include <stdio.h>
#include <stdlib.h>
#define MAX_LINE_LENGTH 512
int exec(const char* in, const char** out, const size_t n)
{
    char buffer[MAX_LINE_LENGTH];
    FILE *file;
    if ((file = popen(in, "r")) == NULL) return 1;
    for (size_t i = 0; i < n && fgets(buffer, MAX_LINE_LENGTH, file) != NULL; i  = 1) out[i] = buffer;
    if (pclose(file)) return 2;
    return 0;
}
#define N 128 
int main(void)
{
    const char* buffer[N];
    const char cmd[] = "<system command to run>";
    const int code = exec(cmd, buffer, N);
    for (int i = 0; i < N; i  = 1) printf("%s", buffer[i]);
    exit(code);
}

While there were plenty of redundancies with what I wrote that are now fixed, this still causes a segmentation fault at runtime.

CodePudding user response：

Focusing on the edited code, this assignment

out[i] = buffer;

has problems.

In this expression, buffer is implicitly converted to a pointer-to-its-first-element (&buffer[0], see: decay). No additional memory is allocated, and no string copying is done.

buffer is rewritten every iteration. After the loop, each valid element of out will point to the same memory location, which will contain the last line read.

buffer is an array local to the exec function. Its lifetime ends when the function returns, so the array in main contains dangling pointers. Utilizing these values is Undefined Behaviour.

Additionally,

for (int i = 0; i < N; i  = 1)

always loops to the maximum storable number of lines, when it is possible that fewer lines than this were read.

A rigid solution uses an array of arrays to store the lines read. Here is a cursory example (see: this answer for additional information on using multidimensional arrays as function arguments).

#include <stdio.h>
#include <stdlib.h>

#define MAX_LINES 128
#define MAX_LINE_LENGTH 512

int exec(const char *cmd, char lines[MAX_LINES][MAX_LINE_LENGTH], size_t *lc)
{
    FILE *stream = popen(cmd, "r");

    *lc = 0;

    if (!stream)
        return 1;

    while (*lc < MAX_LINES) {
        if (!fgets(lines[*lc], MAX_LINE_LENGTH, stream))
            break;

        (*lc)  ;
    }

    return pclose(stream) ? 2 : 0;
}

int main(void)
{
    char lines[MAX_LINES][MAX_LINE_LENGTH];

    size_t n;
    int code = exec("ls -al", lines, &n);

    for (size_t i = 0; i < n; i  )
        printf("%s", lines[i]);

    return code;
}

Using dynamic memory is another option. Here is a basic example using strdup(3), lacking robust error handling.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

char **exec(const char *cmd, size_t *length)
{
    FILE *stream = popen(cmd, "r");

    if (!stream)
        return NULL;

    char **lines = NULL;
    char buffer[4096];

    *length = 0;

    while (fgets(buffer, sizeof buffer, stream)) {
        char **reline = realloc(lines, sizeof *lines * (*length   1));

        if (!reline)
            break;

        lines = reline;

        if (!(lines[*length] = strdup(buffer)))
            break;

        (*length)  ;
    }

    pclose(stream);

    return lines;
}

int main(void)
{
    size_t n = 0;
    char **lines = exec("ls -al", &n);

    for (size_t i = 0; i < n; i  ) {
        printf("%s", lines[i]);
        free(lines[i]);
    }

    free(lines);
}