C strange error with strcpy and even stranger solution

Question

I'm creating a program to open .txt files in a given directory, I have an array with all the absolute paths of the files inside the directory in question and I'm creating a function to extract and return the name of the files, the function is written as follows:

char *name(char *string) {
    int i = strlen(string);
    char *aux;
    while(string[i-1] != '/'){
        i--;
    }
    strcpy(aux, &string[i]); 
    return aux;
}

The above function is giving a Segmentation Fault error, but if I add the following line " int j = 0;" before the declaration of aux the mistake is gone, the new and working code is

char *name(char *string) {
    int i = strlen(string);
    int j = 0;
    char *aux;
    while(string[i-1] != '/'){
        i--;
    }
    strcpy(aux, &string[i]); 
    return aux;
}

input: C:\test\a.txt
output: a.txt

Why the addition of "int j = 0;" solves the problem? I'm stuck with that and can't continue because I don't know if this inconsistency might lead to bigger problems later, I'm thinking about writing my own function to copy the strings, but before that I really want to understand that error.

another solution that I found is interchange the order of the declarations "int i = strlen(string);" and "char *aux;" — user1493813, Jul 01 '12 at 04:06
@user1493813, These aren't solutions. This is undefined behaviour. — chris, Jul 01 '12 at 04:11
Also, another side note, your input is specifying backslashes, and your loop is looking for forwardslashes. You don't have any loop terminator other than finding a forward slashes, so you should segfault on this input as well. To be safe, put in a check to bail on i == 0. — Nick, Jul 01 '12 at 04:14
Props for "can't continue because i don't know if this inconsistency might lead to bigger problems later" and coming here to ask instead of just carrying on after finding something that "works". — Daniel Fischer, Jul 01 '12 at 04:18
Nick, i mistyped the input (just copied it from windows bar), the real input is using forwardslashes — user1493813, Jul 01 '12 at 04:23

houbysoft · Accepted Answer · 2012-07-01T04:23:11.673

4

You never allocate aux. aux needs to point to a valid memory location before you attempt to copy anything to it.

Instead of char *aux, you need something like char *aux = malloc(i+1);. Note that i+1 is overkill because in your case aux will always be at least 3 characters shorter than string (it won't contain C:\), but you probably don't care for such small strings. Remember to free() the pointer once you're done with it.

Also, the reason you found it works by switching orders of declarations and/or adding a declaration is probably that you got lucky and somehow the location to which aux points to is valid (if you do just char *aux;, aux points to a random location). This is pure luck however, and is still invalid code even though it seems to work.

In the future, you might want to use a tool like Valgrind to help you diagnose memory problems. You should also read a tutorial on basic memory management and pointers in C.

edited Jul 01 '12 at 04:23

answered Jul 01 '12 at 04:05

houbysoft

32,532
24
103
156

1

strcpy copies the null terminator, you absolutely need (i+1). – Nick Jul 01 '12 at 04:10
@Nick: no you don't. `aux` is always shorter than `string`. – houbysoft Jul 01 '12 at 04:11
@Nick: added explanation to answer in case it wasn't obvious. – houbysoft Jul 01 '12 at 04:13
Yeah I missed the part that he was always using absolute paths, thanks. – Nick Jul 01 '12 at 04:16
many thanks, i've use a counter in the loop to count the numbers of characters and then i do " char *aux = malloc(cont + 1); ", now i realize that it was such a dumb pointer mistake, i really need to pay attetion to the basics – user1493813 Jul 01 '12 at 04:21
@user1493813: glad it helped. You can mark it as the "accepted" answer using the green checkmark on the left. And yes, using a counter is the "proper" solution :) – houbysoft Jul 01 '12 at 04:25

score 3 · Answer 2 · answered Jul 01 '12 at 04:22

Since it sounds like you are only interested in utilizing the filename portion of the string as parameter, another option is to use the portion of the string you already have.

Try: aux = &string[i]; instead of the strcpy.

This gives you a pointer into the part of the string you are interested in (namely, the last part after the final '/').

Secondly, ensure you have a '/' in all of your input strings, otherwise bad things will happen (i.e. your loop will go past the beginning of the string, likely encountering a segmentation fault at some point). It would be best to put a condition on the loop so that it doesn't continue beyond i = 1.

score 1 · Answer 3 · answered Jul 01 '12 at 04:08

1

You don't allocate any memory to aux. You are trying to write to memory through an uninitialized pointer.

answered Jul 01 '12 at 04:08

Nick

659
4
8

C strange error with strcpy and even stranger solution

3 Answers3