How can I use pointers received from a function correctly? (in C)

Question

I've written a function called safecat that adds one string (called ct) to the end of another string (called s). The resulting new s is then returned.

In the main function my task is to check if my function worked the intended way, and if so, then print the result of my function safecat.

The problem I have is that when I assign the return value of safecat to another char-string (in this case str) in my main function, the stuff in str which comes from ct is just garbage. I don't understand where the problem is, if I just do printf("%s", safecat(s, ct)); I get the correct result.

Here you see my code:

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

char *safecat(char *s, const char *ct);

int main()
{
    char s[] = "Twin";
    const char ct[] = "Peaks";
    char *str = safecat(s, ct);
    if(str == NULL){
        printf("Error in function!");
        return 1;
    }
    printf("\n%s\n", str);
    return 0;
}

char *safecat(char *s, const char *ct){
    int i, k, j = 0;
    int max_count = strlen(s) + strlen(ct) + 1;
    for(i = strlen(s); i < max_count; i = i + sizeof(char)){
        *(s + i) = (char *) malloc((strlen(s) + strlen(ct) + 1) * sizeof(char));
        if(!(s + i)){
            for(k = strlen(s) / sizeof(char); k < i; k++){
                free(*(s + k));
            }
            return NULL;
        }
        *(s + i) = *(ct + j);
        j++;
    }
    return s;
}

I think the error happens when I assign safecat to str. When I print out str I get "TwinP' a" instead of "TwinPeaks".

Thanks for helping!

Answer 1

You can not change the size of the array

char s[] = "Twin";

in the function using malloc.

And in any case this loop

for(i = strlen(s); i < max_count; i = i + sizeof(char)){
    *(s + i) = (char *) malloc((strlen(s) + strlen(ct) + 1) * sizeof(char));
    if(!(s + i)){
        for(k = strlen(s) / sizeof(char); k < i; k++){
            free(*(s + k));
        }
        return NULL;
    }
    *(s + i) = *(ct + j);
    j++;
}

does not make sense. For example the expression *(s + i) has type char instead of the type char *. And also it is not clear why a memory is allocated in each iteration of the loop.

A correct approach is to allocate dynamically a new array with the size equal to the sum of the sizes of the source arrays plus one and to copy the source arrays in the allocated array.

Here is a demonstrative program that shows how it can be done. Also you should free the allocated memory when the array is not needed any more.

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

char * safecat(const char *s1, const char *s2)
{
    char *result = malloc(strlen(s1) + strlen(s2) + 1);

    if (result != NULL)
    {
        char *p = result;

        while (*s1) *p++ = *s1++;
        do { *p++ = *s2; } while (*s2++);
    }

    return result;
}

int main( void )
{
    char s[] = "Twin";
    char ct[] = "Peaks";

    char *str = safecat(s, ct);

    if (str == NULL) 
    {
        puts("Error in function!");
        return 1;
    }

    puts(str);

    free(str);

    return 0;
}

The program output is

TwinPeaks

Of course you could use standard string functions strcpy and strcat instead of the loops that can be both written even in the return statement

return result == NULL ? result : strcat( strcpy( result, s1 ), s2 ); 

Answer 2

When you copy the new string to the old string you wind up putting in the null character after the initial P. What you need to do is as follows.

1. malloc a new buffer (ns) of the size of maxcount.

2. strcpy s into ns

3. strcpy ct into ns + strlen(s)

Note. If you are not allowed to used strcpy(), then write your own version as a function safecpy() in the a similar fashion to safecat()

4. return ns

Note that you would want to free ns sometime later in the program if you no longer need it.

Answer 3

I think that you misunderstand the meaning of malloc.

You are using malloc as if it would make a new slot in or at the end of a string. malloc(sizeX), however, reserves a new memory block with sizeX bytes somewhere in the memory (rather than at a particular position that you could determine). And the result of malloc is a pointer to a memory block, not a character.

So what you do with expression *(s + i) = (char *) malloc((strlen(s) + strlen(ct) + 1) * sizeof(char)); is actually writing a pointer value (usually something obscure when viewed as characters) as value directly into string s at the position of s's string terminating character;

For your safecat, first reserve memory for the new concatenated result string, then fill it up with your logic (I suppose this is a programming assignment and you are not allowed to use strcpy, right?

int slen = strlen(s);
int ctlen = strlen(ct);
char *result = (char *) malloc((slen + ctlen + 1) * sizeof(char));
for (int i=0; i<slen; i++)
  result[i] = s[i];
for (int i=0; i < ctlen + 1 ; i++) // note "i < ctlen + 1" for considering string terminator of ct.
  result[i+slen] = ct[i];

Answer 4

Thank you to everyone who helped. It was a coding assignment and I was able to figure it out the next day.

The stuff I've written must have seemed to be very chaotic as I've just learned about pointers, and yes, I was not allowed to use 'strcat()' or 'strcpy()'.