Segmentation fault- strcat

Question

This is my code:

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

void main(int arge, char *argv[])
{
    FILE *f1;
    char ch,*fn="~/lyrics/";
    strcat(fn,argv[1]);
    strcat(fn,".txt");
    if( (f1 = fopen(fn,"r"))==NULL )
    {
        printf("\nWrong filename\n%s not found",argv[1]);
        return;
    }
    while((ch=getw(f1))!=EOF)
    {
        printf("%c",ch);
    }
}

I compiled it using gcc -g -o file file.c and the compiler gave no error messages. But when I run it I get the error message:

Segmentation fault (core dumped)
Bad permissions for mapped region at address 0x8048659 at 0x402C36B: strcat 
(in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) by 0x80484D6: main (lyrics.c:9)

Can anyone please help me?

@SuiciDoga: Please, what isn't working. And how does it fail? — alk, Mar 30 '16 at 14:18
You might want to read the answers below, which explain the issue with the question's code. @SuiciDoga — alk, Mar 30 '16 at 14:35

score 11 · Accepted Answer · answered Dec 16 '12 at 12:19

You don't have enough space in fn. By strcat'ing on to it you overwrite the end of its stack allocation and into the stack .. hence the segmentation fault.

You could try the following instead:

char fn[255];
strcpy( fn, "~/lyrics/" );
strcat( fn, argv[1] );
strcat( fn, ".txt" );

You just have to be sure that the whole path and filename can fit into 255 characters.

Alternatively you could do this:

char* fn = NULL;
int argvLen = strlen( argv[1] );
fn = malloc( 9 + argvLen + 4 + 1 ); // Add 1 for null terminator.
strcpy( fn, "~/lyrics/" );
strcat( fn, argv[1] );
strcat( fn, ".txt" );

And you have definitely allocated enough space for the string. Just don't forget to free it when you have finished with it!

Your answer is misleading to not say wrong. This "*You don't have enough space in fn.*" is **not** the root cause of the segmenation violation. The root-cause is that you cannot copy memory to a read-only memory location, which `fn` points to, as `"~/lyrics/"` is a string literal which cannot be overwritten. — alk, Jan 16 '15 at 20:15

score 6 · Answer 2 · answered Dec 16 '12 at 12:19

6

char *fn = "~/lyrics/";

Because fn could point on a string in read-only memory, you should declare fn as pointer to const char.

const char *fn = "~/lyrics/";

Then you can see there are some errors. Here is a better solution:

char fn[MAX_SIZE] = "~/lyrics/";

Here MAX_SIZE shall be the sum of the size of "~/lyrics/", the maximum length of argv[1] and the length of ".txt".

answered Dec 16 '12 at 12:19

md5

23,373
3
44
93

Take care that `MAX_SIZE` provides room for the `0`-terminator, a C-"string" relies on. The `sizeof` `"~/lyrics/"` is 8 for the characters plus 1 for the `0`-terminator. – alk Jan 16 '15 at 20:16

score 1 · Answer 3 · answered Dec 16 '12 at 12:26

This approach is not portable.

If using glibc you also might call asprintf() which just allocates as much memory as you need.

#include <stdio.h>

...

char * pFn = NULL;

if (-1 == asprintf(&pFn, "~/lyrics/%s.txt", argv+1);
{
  perror("asprintf()");
}
else
{
  ... /* use pFn */
}

free(pFn);

...

score 0 · Answer 4 · answered Mar 05 '23 at 07:58

0

I think a better way to do this is by making a function like:

#include <string.h>

char* joinString(char* s1, char* s2) {
 char s[sizeof(s1) + sizeof(s2) + 1];
 strcat(s, s1);
 strcat(s, s2);
 return s;
}

answered Mar 05 '23 at 07:58

Incarnator

1

1

This solution is incorrect. Arrays decay to pointers in function parameters. `sizeof()` on a pointer then gives the size of the pointer, and not the pointed to data. – Harith Mar 05 '23 at 09:21

Segmentation fault- strcat

4 Answers4

Linked