-2

I'm getting a segmentation fault when I call strcat(); however, I've already malloc'd the destination string and initialized the previous string. This is in an assignment to make a shell in C, I'm just getting tripped up on this error. Here's what the code looks like for this particular segment. To note, this is a part of a larger program so I only included the bits I thought were relevant.

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

int main(int argc, char* argv[])
{
    char* token, *file_name, *srt_cmm, *str_chr1;
    str_chr1 = "<";
    file_name = (char*)malloc(100);
    srt_cmm = (char*)malloc(500);
    srt_cmm = "sort ";
    


    if(strstr(token, str_chr1) != NULL)
    {
        token = strtok(NULL, " ");
        file_name = token;
        strcat(srt_cmm, file_name);
        execvp("sort", &srt_cmm);
    }

    free(file_name);
    free(srt_cmm);
}

The first thing I did was allocate both of the variables more space in an attempt to see if that'd fix it. Other than that I just put printf statements in to see where it was getting hung up, it clears the file_name assignment but throws the error at the strcat(). I've checked, and file_name is being populated by the tokenized string.

Pdom
  • 1
  • 2
  • 1
    You're trying to `strcat()` to a static string, undefined behaviour. Did you mean to add `"sort "` to your allocated `srt_cmm` buffer instead? – tadman Feb 28 '23 at 02:40
  • PSA: Don't [cast `malloc()` in C](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). – tadman Feb 28 '23 at 02:41
  • 2
    You need to learn up more on what `malloc()` does, and how you're leaking memory all over here by losing track of your allocations. `file_name` is allocated, then abruptly stomped with the value in `strtok()`. Remember, in C `x = y` does **NOT** copy C string data, it just copies the pointer. – tadman Feb 28 '23 at 02:42
  • @tadman Ok, so I removed the casts, and then I added a strcpy() to put "sort " into srt_cmm at the start of the program. Strcat runs successfully now, I just have other problems to tend to. In regards to malloc(), when you say that the allocated space is stomped with the value, is it because I give it a new pointer to some data and the space is now inaccessible so it's leaking? Thanks for the helpful response. – Pdom Feb 28 '23 at 02:52
  • When you `malloc()` you're obligated to hang on to that until you release it with `free()`, and `free()` can **only** release pointers created via `malloc()` (or equivalent, like `calloc()`). Here you effectively `free("sort ")` which is invalid. – tadman Feb 28 '23 at 03:02
  • Treat the result from `malloc()` like your coat check ticket. If you lose it, you will never get that jacket back. This is why tracking ownership of these allocations is important, and you'll need to have some part of your code responsible for that even if the data is being used in other places. Tools like [Valgrind](http://valgrind.org) can help identify mistakes like this. – tadman Feb 28 '23 at 03:03
  • You need to correct your sample code as it does not even compile and include the required include files. When I tried, the compiler complained that variable "str_chr1" was not defined. You cannot assume for example that someone is going to know that the "execvp" function is in the "unistd.h" file. – NoDakker Feb 28 '23 at 03:10

1 Answers1

1

Compiling your code with gcc -Wall t.c produces:

t.c: In function ‘main’:
t.c:17:8: warning: ‘token’ is used uninitialized [-Wuninitialized]
   17 |     if(strstr(token, str_chr1) != NULL)
      |        ^~~~~~~~~~~~~~~~~~~~~~~
t.c:9:11: note: ‘token’ was declared here
    9 |     char* token, *file_name, *srt_cmm, *str_chr1;
      |           ^~~~~

which is the first (of many) problems with your code.

The second obvious problem is that strtok(NULL, " "); without a previous call to strtok with non-NULL argument makes no sense.

There are more problems (such as a memory leak when re-assigning srt_cmm).

Employed Russian
  • 199,314
  • 34
  • 295
  • 362
  • You're correct, in adding the code I neglected to add everything required. I've mostly figured out this issue with the help of an earlier commenter. I'll just mark this as solved and fix the rest of it. Thank you for your answer. – Pdom Feb 28 '23 at 03:54