views:

702

answers:

3

In the code below, the line:

*end = *front;

gives a segmentation fault. I asked a similar question here but I'm not sure if this is because I have two copies of num. Please explain why it's seg-faulting. Thank you.

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

char* getPalin(char* num);

int main()
{
    char* num = (char*)malloc(100);

    num = "123456";

    printf("%s\n", getPalin(num) );

    return 0;
}

char* getPalin(char* num)
{
    int length = strlen(num);

    if ( length % 2 == 0 )
    {
     char* front = num;
     char* end = num + strlen(num) - 1; //pointer to end

     while( front != num + (length/2) ) //pointers not middle yet
     {
      *end = *front;

      printf("%c", *end);

      front++;
      end--;
     }
    }

    return num;
}
+4  A: 

Use

strncpy(num, "123456", 100);

instead of

num = "123456";
Konstantin
sorry, not sizeof(num), but the size of the allocated memory, that is 100. I.e. strncpy(num, "123456", 100);
Konstantin
OP asked for explanation but you gave just a solution. And never forget to manually add null-terminator after strncpy because it is not guaranteed that strncpy adds it. I know it is not relevant in this example because "123456" is shorter than 100 bytes but if you decided to use strncpy rather than strcpy, use it properly - you avoided one potential problem (buffer overrun) but introduced other one (unterminated string).
qrdl
@qrdl - As long as the destination size specified in strncpy is larger than the string length, strncpy will automatically pad out with null characters. It is only if the destination size is specifed as being less than the string length to be copied that the null terminator may be missed. Another piont is to remember that strncpy will NOT check that there is sufficient space in the destination memory. So I suppose strncpy(num,"123456", sizeof(num)) would be more correct.
ChrisBD
num is a "char*" so sizeof(num) will be 4 (or 8 if you've got better hardware than me).
paxdiablo
@ChrisBD - Read my comment more carefully. I wrote that I know that it is not important in this case. And just FYI - sizeof(num) is sizeof(char *) in this case because num is declared as char* in parameter list.
qrdl
Yes you're quite right about the sizeof. It would have been okay if num was defined as char num[100]; In this instance there is no way of programatically determining the amount of memoery allocated by the malloc.
ChrisBD
+10  A: 

These two lines:

char* num = (char*)malloc(100);
num = "123456";

have the following effect.

The first allocates 100 bytes and sets num to point to those bytes.

The second changes num to point to the string "123456" which is almost certainly in read-only memory.

Any attempt to change the contents of read-only memory will result in a segmentation violation. You need to copy the string into the malloc'd num before attempting to change it, with:

strcpy (num, "123456");

That's the line you should have where you currently have:

num = "123456";
paxdiablo
+1  A: 

As per Konstantin's answer.

You have already allocated memory for num with the malloc statement.

If you hadn't then you could get away with:

char* num = "123456";

Which would define and allocate memory on the fly but it would most likely be allocated as a constant and thus read only.

Using strncpy rather than strcpy to copy "123456" will ensure that any extra space beyond the end of string null terminator is also initialised to null, as long as you specify n as being 100 (for your example). Otherwise without initialising the memory allocated by malloc to null (memset(num, 0, 100)) then it is conceivable that you could step beyond the end of the string.

Oh almost forgot. It is advisable to use strcpy_s or strncpy_s as they are more secure, although for your code it won't matter.

ChrisBD
I actually consider those two _s() functions to be a crutch for people who don't know how to code defensively [i.e., check preconditions first - I've never even found the need to use strncpy() and still my code is as "secure" as any using the _s() variants]. On top of that, they're not part of the standard yet.
paxdiablo
Hence the reason why I said advisable.It never ceases to amaze me how much commercial software leaves itself open to buffer overrun attacks.
ChrisBD