[SOLVED] Safe strcpy and strcat in C

nsb · 2011-07-14 14:05:40

Hello

I wanted to make a new implementation of the memoir program in C. I use C quite a lot for programming code for simulations, but with string I never had much to do. I read, that strcpy and strcat are quite unsafe, so I thought to write a wrapper for them. But now I get some memory errors, which I don't quite understand. Could you have a look at it? The files are utils.c, utils.h and test.c. The output of valgrind is in valgrind.out.

Does anybody has an idea, what the problem is?

Regards
nsb

Last edited by nsb (2011-07-14 21:22:30)

falconindy · 2011-07-14 14:51:41

strcat and strcpy already have safe cousins -- strncat and strncpy.

The problem with your code is that you're not allocating enough memory.

int length = strlen(from) + 1;
*to = (char *) xrealloc(*to, length * sizeof(char));

Here, you're reallocating *to as the size of from, but you really want (strlen(*to) + strlen(from) + 1) bytes.

Last edited by falconindy (2011-07-14 14:53:42)

nsb · 2011-07-14 15:37:57

I know about strncat and strncpy, but I wanted to have functions, that pretty much by themself, without me telling them, how many character they should copy.

Further the size you point out is from the cpy function. There I only should need to allocate space of the size from the source, right?

falconindy · 2011-07-14 15:59:47

My static code analysis sucks without coffee. You're not allocating any memory for the pointers in your test program. There's some other cleanup you'll need to do as well...

nsb · 2011-07-14 17:10:27

Yeah, without coffee coding is impossible
Anyway, the pointers should get some memory in the cpy/cat function. realloc(NULL, size) is equivalent to malloc(size). Or am I wrong?

falconindy · 2011-07-14 17:25:12

Well, somewhere along the way, they don't. The realloc call is where its crashing.

nsb · 2011-07-14 18:00:34

Sorry, I should have said more: I know, that it crashes the first time I call cpy and that it crashes while calling realloc through xrealloc, but I really don't understand why. The length of the source string ("from") is recognised, so I don't understand, how realloc can fail.

tavianator · 2011-07-14 19:17:15

- You don't initialize "second" or "third" in main(), so you are calling realloc(<undefined value>, ...) at some point. Set them to NULL in main(). This causes the first set of valgind errors.

- In cat(), line 52, when you call "cpy(to, buffer);", cpy() realloc's "to" back down to a smaller size. Thus, you make it big enough, then immediately shrink it. "buffer" isn't necessary anyway, because realloc copies the data when it has to. Deleting lines 46-49 and 52 makes that function work. This causes the second set of valgrind errors.

- What's up with the

errno = 0;
if (<error condition>) {
  error(0, ...);
}
if (errno) {
  error(0, ...);
}

pattern? Obviously errno is 0, since you just set it. And error(0, ...) won't exit your program.

- addslash() is broken, since cat(&path, "/") can set path to a different value, but the caller has no way to know where it went. IOW,

char *path = NULL;
cpy(&path, "/home");
addslash(path); /* does realloc(path, 6), but path still has the old value! */
printf("%s\n", path); /* undefined behaviour, path points to invalid memory */

- Check out strdup() for a standard version of cpy()

- You should free() "first" and "second".

nsb · 2011-07-14 21:22:09

Thanks for your answers!

- It was indeed the thing with the unitialized pointers.

- I thought, that every function would change the global variable errno to something different than 0. To make sure, that I catch the error of a particular function, I first set errno to 0. You are right, that error(0, ...) does not exit the program, I should change that.

- addslash is a leftover, I should abandon that, since a simple cat(&path, "/") is equally fast to write.

- thanks for the tip with strdup

Regards
Noah

tavianator · 2011-07-14 22:23:10

You're welcome!

nsb wrote:

- I thought, that every function would change the global variable errno to something different than 0. To make sure, that I catch the error of a particular function, I first set errno to 0. You are right, that error(0, ...) does not exit the program, I should change that.

    pointer = malloc(size);
    errno = 0;
    if(pointer == NULL){
        error(0, 0, "Could not allocate enough space!");
    }
    if(errno){
        error(0, errno, strerror(errno));
    }

is clearly not helpful, because you reset errno after you called the function. And "pointer == NULL" is enough to catch any malloc() error anyway. Similarly for realloc(). As well, strcat() and strcpy() never fail, so you could simply write strcpy/strcat(*to, from) and save yourself both useless error condition checks.

nsb · 2011-07-15 09:27:59

Sure, I first only thought about the cat and cpy function. In the xmalloc and xrealloc function I really did it wrong .
Ok, so I will leave them away.

juster · 2011-07-17 13:48:22

errno problems were covered. Here are comments on your usage of error(3).

void *
xmalloc(size_t size){
    void * pointer;
    pointer = malloc(size);
    errno = 0;
    if(pointer == NULL){
        error(0, 0, "Could not allocate enough space!");
    }
    if(errno){
        error(0, errno, strerror(errno));
    }
    return pointer;
}

The first parameter of error(3) is a program status. If set to non-zero, after printing the error message, the program will exit, with the exit code set to the first parameter. Your program will never exit on error because the first parameter is 0. This seems like unwanted behavior.

Using strerror(errno) in the second call of error is redundant. If the second parameter is a non-zero value, the error function will automatically print strerror(errno) to the terminal. The third parameter should be [the printf style format of] your custom error message.

tav: what do you mean that checking the returned pointer for NULL is not enough to check for errors? Is there a case where an error occurs, errno is set, and malloc does not return NULL?

tavianator · 2011-07-18 16:36:30

juster wrote:

tav: what do you mean that checking the returned pointer for NULL is not enough to check for errors? Is there a case where an error occurs, errno is set, and malloc does not return NULL?

I said it is enough .

juster · 2011-07-19 01:41:43

Oh. Whoops!

nsb · 2011-07-19 11:28:26

Thanks. I already implemnted it that way. If you want to have look on the finished program: click here!

enjoy!
nsb

Arch Linux

#1 2011-07-14 14:05:40

[SOLVED] Safe strcpy and strcat in C

#2 2011-07-14 14:51:41

Re: [SOLVED] Safe strcpy and strcat in C

#3 2011-07-14 15:37:57

Re: [SOLVED] Safe strcpy and strcat in C

#4 2011-07-14 15:59:47

Re: [SOLVED] Safe strcpy and strcat in C

#5 2011-07-14 17:10:27

Re: [SOLVED] Safe strcpy and strcat in C

#6 2011-07-14 17:25:12

Re: [SOLVED] Safe strcpy and strcat in C

#7 2011-07-14 18:00:34

Re: [SOLVED] Safe strcpy and strcat in C

#8 2011-07-14 19:17:15

Re: [SOLVED] Safe strcpy and strcat in C

#9 2011-07-14 21:22:09

Re: [SOLVED] Safe strcpy and strcat in C

#10 2011-07-14 22:23:10

Re: [SOLVED] Safe strcpy and strcat in C

#11 2011-07-15 09:27:59

Re: [SOLVED] Safe strcpy and strcat in C

#12 2011-07-17 13:48:22

Re: [SOLVED] Safe strcpy and strcat in C

#13 2011-07-18 16:36:30

Re: [SOLVED] Safe strcpy and strcat in C

#14 2011-07-19 01:41:43

Re: [SOLVED] Safe strcpy and strcat in C

#15 2011-07-19 11:28:26

Re: [SOLVED] Safe strcpy and strcat in C

Board footer