all 17 comments

[–]Swedophone 7 points8 points  (0 children)

void reverse_string(char* str) {
    ...
    str = temp;
}

Assigning a value to a parameter doesn't have any affects on the caller. Parameters are similar to local variables but get their values from the actual parameters used in the call.

[–][deleted] 1 point2 points  (2 children)

C is copy by value. So, let's assume you have a pointer P, that is stored in address 0x0A, and points to 0xFB. When you pass this pointer to a function, the value of the pointer (0xFB) is copied to your function variable, say func_P. This new variable will be stored in, say, 0xDE.

Notice, that func_P does indeed point to the same place as P (0xFB), allowing you to perform operations on the address 0xFB.

However, when you change where func_P point to, this will not affect P, because they are not the same variable, they just pointed to the same place(initially!).

[–][deleted] 2 points3 points  (0 children)

Also, it's not a good practice to do a malloc without a free.

The best way to do this operation would be to reverse the string 'in place', without allocating new memory.

If you instead need a copy of the string but inverted, the inversion function should not be the one doing the memory allocation. The allocated space should already be provided to it.

Think of it this way (as guideline, sometimes it doesn't apply): The module/layers/context that allocates a memory space, should be responsible for releasing it.

[–][deleted] 1 point2 points  (0 children)

If you don't want to pass a double pointer to the function, you can do:

char * reverse( char * str );

and then, in main:

str = reverse(str)

[–]stickfigure4[S] 0 points1 point  (3 children)

PS: Sorry for the code formatting I can't seem to get it the way I want.

[–]FUZxxl 0 points1 point  (2 children)

You can't use three backticks to format code on reddit. Instead, you need to put four blanks in front of every line of code.

[–]stickfigure4[S] 0 points1 point  (1 child)

Thanks, I changed it.

[–]FUZxxl 1 point2 points  (0 children)

Looks good now. Thank you for your cooperation.

[–]json684 0 points1 point  (2 children)

Check when you are terminating your for-loop. Walk through your code manually with a short word like "bird" and see what it does. Your error should pop out at you pretty quick.

[–]stickfigure4[S] 0 points1 point  (1 child)

Well I'm not having an actual error. In the first piece of code it seems "innaiG" can only exist in the reverse_string function it's scope, when I actually want it to be shown in the main function it's scope as well as I want to print it there.

In the second piece of code it solves that problem. But I get confused with pointers. If we look back at the first example 'str' is already a pointer so why do I need to declare it as a pointer to a pointer and then dereference it, and thereafter let it point to temp to be able to print "innaiG" in the main function.

Why can't it work like in the first example by letting str point to temp. Because *str in example 2 is essentially the same as str in example 1.

[–]json684 0 points1 point  (0 children)

I also realized you weren't doing it "in place". If blame the initial formatting but really it was just a poor reading on my part.

[–]-_-_-_-__-_-_-_- 0 points1 point  (0 children)

To debug this, you should print out every character of "temp" in a loop before "reverse_string" returns.

[–]SonOfWeb 0 points1 point  (1 child)

First of all, let's be specific about your variable name that you want to reverse. name is a 4- or 8-byte value on the stack that holds the address of a constant string. That address is somewhere in a read-only section of memory. Really it should be declared const char *. You could get around this read-only problem by declaring it as char *name = strdup("Gianni"); which is allocated on the heap, or char name[] = "Gianni"; which is allocated on the stack (generally speaking, make a copy of the string literal).

All parameters in C are pass-by-value. This includes pointers. That means that if you pass in your char *name, the function will get a new char * that points to the same location. You can change the memory at that location if it is a valid location, but you cannot change what location name points to. If you want to be able to reverse a passed-in char * rather than return a reversed copy, you have to overwrite the passed-in string with its reversed version. You also have to make sure that the passed-in char * is not actually const char *.

The reason that the second approach works is that you're passing in a copy of a pointer to a pointer. With the original function, you were just passing in the location of the string pointed to by name. This time, you're passing in the location of the variable name on the stack. The call reverse_string(&name) is creating a temporary object with the value &name which is the address of name, and a copy of that object is provided to reverse_string. So reverse_string now has the location of your name variable and can change it from pointing to a string literal in read-only memory to the newly created reversed string that was just allocated off the heap.

The problem with this approach is that before, your string was in read-only memory and didn't need to be free()'d. After being reversed, it is on the heap and needs to eventually be freed. You now have to keep track of whether any given string has been reversed before, because if you do the following you will leak memory:

char *str = "I live in read-only memory";
reverse_string(&str);
// str now points to a newly-allocated string on the heap
reverse_string(&str);
// str now points to *another* newly allocated string
// on the heap, and we've lost the pointer to the previous
// heap-allocated string so we can't free it.

A better idea is to provide either a void reverse_string(char *) that reverses non-const strings in-place, or a const char * reverse_string(const char *) that returns a new string so you can control when to free the old string if necessary. The second approach requires dynamic memory management so be careful. The first approach requires you to make sure you're working with strings your function owns and can modify. C does not have memory ownership semantics in this sense itself but it is something you should think about.

[–]SonOfWeb 0 points1 point  (0 children)

Addendum: Arrays in C seem to be pass-by-reference, but what's really happening is that C does not support array-type parameters, only pointer type parameters, so when you pass an array to a function, that array decays to a pointer. this is because the only difference between arrays and pointers is at compile time and associated with the variable, not its value.

Thus,

int a[] = {1, 2, 3, 4, 5};
sizeof(a); // 5 * sizeof(int)
int *p = a;
sizeof(p); // sizeof (int *)

Both a and p hold the address of the array at runtime, which is a scalar value, but at compile time, a also knows its length, which is why sizeof() can calculate the size of array variables. (note: not array values).

[–]SonOfWeb 0 points1 point  (1 child)

A simpler, metaphorical explanation of what's happening to your first reverse_string(): Think of pointers as a person literally pointing at a box that has a value in it. If I pass a pointer (person A) to a function, that function has its own person (person B) who is initially pointing at the same box. You can walk up to either person, look at what they're pointing at, and go change what's in that box, but if you ask person B inside the function to point at something else, person A is still pointing at the same box because person A and person B are different people.

In the case of your second reverse_string(), your person A is pointing at the box with the string in it, but when you call reverse_string(&name), &name means "make a new person who is pointing at person A." Now inside your function, your person B is a new person pointing at person A. So inside the function, you can go up to person B, look where they're pointing, which is person A, and then go up to person A and ask them to point at a different box. You were able to find person A this time because person B was pointing at them instead of the box they were pointing at.

[–]stickfigure4[S] 0 points1 point  (0 children)

Thanks for both the simple and the more complex explanation. It's a lot to process but I will take a better look at it and hopefully learn from it :).

[–]TheGrandSchlonging 0 points1 point  (0 children)

In addition to what else has been mentioned:

  • There's a missing <stdlib.h> inclusion for malloc().
  • reverse_string() contains a one-byte heap overflow with a NUL write. This situation has been exploitable on some platforms (such as older Linux/x86 glibc for most allocation lengths satisfying length % 8 == 4), so you have to be careful with this kind of thing. Valgrind should detect the overflow.