Search code examples
arrayscexecvp

Executing a program arguments with C and evecvp failing to forward correctly

I am trying to execute a program with the execvp function within an overseer and client distributed system. The client sends a program to be executed with the arguments:

char buf[500];
int bytes_recieved = 0;
char array[1][26];
bytes_recieved = recv(clientfd, buf, 5000, 0);
buf[bytes_recieved] = '\0';

char buf1[50];
int bytes_recieved1 = 0;
char *array1[4];

for (int i = 0; i < 3; i++){
   bytes_recieved1 = recv(clientfd, buf1, 50, 0);
   array1[i] = buf1;
   printf("%s = buffer\n", buf1);
} 
buf1[bytes_recieved] = '\0';

if(bytes_recieved != -1){
   printTime();
   fprintf(stdout,"atempting to execute program: %s\n", buf);
   if(execvp(buf, array1) == -1) {
      return 1;
   }
}

I'm stuck on trying to figure out what happens when I print out the array of arguments in the program the last argument is the same for all of them? for example I run this in the client program to be executed:

./client 12345 home/user/test_program 1 2 3

the result from a simple printf is:

3
3
3

When I manually assign each argument in the array in the overseer:

array1[0] = "1";
array1[1] = "2";
array1[2] = "3";

and send it to the executed program it prints correctly.

I have also tested that the received buffer from the file descriptor is correctly assigning the variables in the array:

 printf("%s = buffer\n", array1[i]);

within the assignment for loop, which returns:

    1 = buffer
    2 = buffer
    3 = buffer

What am I doing wrong? Let me know if you need any more information.

Solution

  • This is some skeletal code, based on your code fragment. I can't test it — you did not provide an MCVE (Minimal, Complete, Verifiable Example — or MRE or whatever name SO now uses) or an SSCCE (Short, Self-Contained, Correct Example).

    char buf[500];
int bytes_received = recv(clientfd, buf, sizeof(buf)-1, 0);
if (bytes_received < 0)
    return 1;
buf[bytes_received] = '\0';

char *array1[5] = { buf };

for (int i = 0; i < 3; i++)
{
    char buf1[50];
    int bytes_received1 = recv(clientfd, buf1, sizeof(buf1)-1, 0);
    if (bytes_received1 < 0)
        return 1;
    buf1[bytes_received1] = '\0';
    array1[i + 1] = strdup(buf1);
    printf("argument = [%s]\n", buf1);
}

printTime();
printf("atempting to execute program: %s\n", buf);
for (int i = 0; array1[i] != NULL; i++)
    printf("argv[%d] = [%s]\n", i, array1[i]);
fflush(0);
execvp(array1[0], array1);
fprintf(stderr, "failed to execute '%s'\n", array1[0]);
return 1;

    Multiple changes include:

    • Using sizeof to determine array sizes.
    • Changing spelling of "receive".
    • Subtracting 1 to allow space for a terminal null byte to convert messages to strings.
    • Making array1 big enough to hold a terminal NULL pointer and initializing (indirectly) the elements after the zeroth to NULL. This is important; the argument array to execvp() must be terminated with a NULL pointer.
    • Return if the initial receive fails, to avoid indexing by a negative number.
    • Making the buf1 array local to the loop; ditto bytes_received1.
    • Return if a subsequent receive fails, to avoid indexing by a negative number.
    • Making a copy of the strings read using strdup() — this is a key change.
    • Not trying to null-terminate buf1 at a position based on the data received in buf.
    • Revising the printing, putting square brackets around the string so trailing spaces or newlines can be spotted more easily.
    • Printing all the arguments to execvp().
    • Debating whether the debug output should go to stderr instead of stdout. I ended up leaving it going to stdout, but that isn't necessarily the best choice.
    • Changing one fprintf(stdout, …) to printf(…) for consistency.
    • Calling fflush(0) to send any pending output to its devices. With the debugging output going to stdout, if the output is piped to another program, the data will be fully buffered, not line buffered, and won't appear unless you force it to. Calling fflush(stdout) would also be an option. It's probable that you shouldn't (don't) have any file streams other than stdin, stdout, stderr open when you're calling execvp().
    • You should consider whether other streams (file descriptors) should be closed before reaching here, perhaps using the O_CLOEXEC or FC_CLOEXEC options to ensure that the file descriptors are closed on successful exec, so that the executed process doesn't get active file descriptors it doesn't know about.
    • Not bothering to check the return value from execvp(). If it returns, it failed; if it succeeds, it doesn't return.
    • Reporting an error message when execvp() fails to execute.
    • Leaving return 1; as part of the code after the failed execvp(). It would often be better to use exit(EXIT_FAILURE); or similar (_exit(EXIT_FAILURE) perhaps). Without the larger context of the function that calls this fragment of a function, it isn't possible to know what's best here.
    • Note that if execvp() fails and returns, rather than exits, you're leaking the memory allocated by strdup(). There should probably be a loop for (int i = 0; i < 3; i++) free(array1[i+1]); to release the copied memory before the return 1;.
    • The code doesn't check that there was no data truncation — it doesn't know if one of the recv() calls would have read more data than it did because there wasn't enough space to store all the data. You'd probably want to check that the actual data size is smaller than the space available to ensure that there wasn't truncation.
    • It isn't clear why the program name can be ten times bigger than the arguments. In general, arguments can be bigger than program names, though, since your sample data has arguments like 1, 2, 3, this is not a problem.