Swapping 2 pointer array of char without using strcpy and must use assignment operator?

Question

Hello i was given a task to fix this program that it generates no errors log generated by valgrind.

This is the recent code

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

char *tukar(){
    char *a, *b, *c;
    a = (char *) malloc(sizeof(char) * 50);
    b = (char *) malloc(sizeof(char) * 50);
    c = (char *) malloc(sizeof(char) * 50);

    strcpy(a,"string a");
    strcpy(b,"string b");

    printf("\nBefore :\n");
    printf("a = %s\n",a);
    printf("b = %s\n",b);

    c = b;
    b = a;
    a = c;

    printf("\nAfter :\n");
    printf("a = %s\n",a);
    printf("b = %s\n",b);

    free(a);
    free(b);
    return c;
}

int main(){
    char *y = tukar();
    printf("%s\n",y);
    free(y);
    getchar();
    return 0;
}

The current errors prints out

==5300== Memcheck, a memory error detector
==5300== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==5300== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==5300== Command: ./aa
==5300== Parent PID: 5110
==5300== 
==5300== Invalid read of size 1
==5300==    at 0x4C30D22: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x4EAA931: puts (ioputs.c:35)
==5300==    by 0x1088CB: main (atan.c:33)
==5300==  Address 0x52010c0 is 0 bytes inside a block of size 50 free'd
==5300==    at 0x4C2ED5B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x108897: tukar (atan.c:26)
==5300==    by 0x1088BB: main (atan.c:32)
==5300==  Block was alloc'd at
==5300==    at 0x4C2DB2F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x1087BF: tukar (atan.c:8)
==5300==    by 0x1088BB: main (atan.c:32)
==5300== 
==5300== Invalid read of size 1
==5300==    at 0x4C30D34: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x4EAA931: puts (ioputs.c:35)
==5300==    by 0x1088CB: main (atan.c:33)
==5300==  Address 0x52010c1 is 1 bytes inside a block of size 50 free'd
==5300==    at 0x4C2ED5B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x108897: tukar (atan.c:26)
==5300==    by 0x1088BB: main (atan.c:32)
==5300==  Block was alloc'd at
==5300==    at 0x4C2DB2F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x1087BF: tukar (atan.c:8)
==5300==    by 0x1088BB: main (atan.c:32)
==5300== 
==5300== Invalid read of size 1
==5300==    at 0x4EB5145: _IO_file_xsputn@@GLIBC_2.2.5 (fileops.c:1309)
==5300==    by 0x4EAA9F2: puts (ioputs.c:40)
==5300==    by 0x1088CB: main (atan.c:33)
==5300==  Address 0x52010c7 is 7 bytes inside a block of size 50 free'd
==5300==    at 0x4C2ED5B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x108897: tukar (atan.c:26)
==5300==    by 0x1088BB: main (atan.c:32)
==5300==  Block was alloc'd at
==5300==    at 0x4C2DB2F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x1087BF: tukar (atan.c:8)
==5300==    by 0x1088BB: main (atan.c:32)
==5300== 
==5300== Invalid read of size 1
==5300==    at 0x4EB515C: _IO_file_xsputn@@GLIBC_2.2.5 (fileops.c:1309)
==5300==    by 0x4EAA9F2: puts (ioputs.c:40)
==5300==    by 0x1088CB: main (atan.c:33)
==5300==  Address 0x52010c6 is 6 bytes inside a block of size 50 free'd
==5300==    at 0x4C2ED5B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x108897: tukar (atan.c:26)
==5300==    by 0x1088BB: main (atan.c:32)
==5300==  Block was alloc'd at
==5300==    at 0x4C2DB2F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x1087BF: tukar (atan.c:8)
==5300==    by 0x1088BB: main (atan.c:32)
==5300== 
==5300== Invalid read of size 1
==5300==    at 0x4C35028: __GI_mempcpy (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x4EB5079: _IO_file_xsputn@@GLIBC_2.2.5 (fileops.c:1327)
==5300==    by 0x4EAA9F2: puts (ioputs.c:40)
==5300==    by 0x1088CB: main (atan.c:33)
==5300==  Address 0x52010c7 is 7 bytes inside a block of size 50 free'd
==5300==    at 0x4C2ED5B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x108897: tukar (atan.c:26)
==5300==    by 0x1088BB: main (atan.c:32)
==5300==  Block was alloc'd at
==5300==    at 0x4C2DB2F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x1087BF: tukar (atan.c:8)
==5300==    by 0x1088BB: main (atan.c:32)
==5300== 
==5300== Invalid read of size 1
==5300==    at 0x4C35038: __GI_mempcpy (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x4EB5079: _IO_file_xsputn@@GLIBC_2.2.5 (fileops.c:1327)
==5300==    by 0x4EAA9F2: puts (ioputs.c:40)
==5300==    by 0x1088CB: main (atan.c:33)
==5300==  Address 0x52010c5 is 5 bytes inside a block of size 50 free'd
==5300==    at 0x4C2ED5B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x108897: tukar (atan.c:26)
==5300==    by 0x1088BB: main (atan.c:32)
==5300==  Block was alloc'd at
==5300==    at 0x4C2DB2F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x1087BF: tukar (atan.c:8)
==5300==    by 0x1088BB: main (atan.c:32)
==5300== 
==5300== Invalid free() / delete / delete[] / realloc()
==5300==    at 0x4C2ED5B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x1088D7: main (atan.c:34)
==5300==  Address 0x52010c0 is 0 bytes inside a block of size 50 free'd
==5300==    at 0x4C2ED5B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x108897: tukar (atan.c:26)
==5300==    by 0x1088BB: main (atan.c:32)
==5300==  Block was alloc'd at
==5300==    at 0x4C2DB2F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x1087BF: tukar (atan.c:8)
==5300==    by 0x1088BB: main (atan.c:32)
==5300== 
==5300== 
==5300== HEAP SUMMARY:
==5300==     in use at exit: 50 bytes in 1 blocks
==5300==   total heap usage: 5 allocs, 5 frees, 2,198 bytes allocated
==5300== 
==5300== 50 bytes in 1 blocks are definitely lost in loss record 1 of 1
==5300==    at 0x4C2DB2F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5300==    by 0x1087CD: tukar (atan.c:9)
==5300==    by 0x1088BB: main (atan.c:32)
==5300== 
==5300== LEAK SUMMARY:
==5300==    definitely lost: 50 bytes in 1 blocks
==5300==    indirectly lost: 0 bytes in 0 blocks
==5300==      possibly lost: 0 bytes in 0 blocks
==5300==    still reachable: 0 bytes in 0 blocks
==5300==         suppressed: 0 bytes in 0 blocks
==5300== 
==5300== For counts of detected and suppressed errors, rerun with: -v
==5300== ERROR SUMMARY: 27 errors from 8 contexts (suppressed: 0 from 0)

i'm suspecting that these assignments

c = b;
b = a;
a = c;

creates the problem. However, my teacher says it cannot be changed or replaced with strcpy(). I'm thinking about replacing the pointer's address but the pointer has been declared malloc() in it.

Is there any way to modify the code without replacing the assignments?

PS : 'tukar' means swap.

Answer 1

Just got back asking from the friend.. He said the return c isn't having a problem. He said also the same about c doesn't need to be malloc()-ed (thanks to unwind's reference)...and about free(a) causes memory leak to c (thanks to Lundin's reference).

free(a) causes c to be gone, which it's reference to b also affected. The revised program will be

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

char *tukar(){
    char *a, *b, *c;
    a = malloc(50);
    b = malloc(50);

    strcpy(a,"string a");
    strcpy(b,"string b");

    printf("\nBefore :\n");
    printf("a = %s\n",a);
    printf("b = %s\n",b);

    c = b;
    b = a;
    a = c;

    printf("\nAfter :\n");
    printf("a = %s\n",a);
    printf("b = %s\n",b);

    free(b);
    return c;
}

int main(){
    char *y;
    y=tukar();
    printf("%s",y);
    free(y);
    getchar();
    return 0;
}

while valgrind gives output

==2527== Memcheck, a memory error detector
==2527== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==2527== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==2527== Command: ./aa
==2527== Parent PID: 2159
==2527== 
==2527== 
==2527== HEAP SUMMARY:
==2527==     in use at exit: 0 bytes in 0 blocks
==2527==   total heap usage: 4 allocs, 4 frees, 2,148 bytes allocated
==2527== 
==2527== All heap blocks were freed -- no leaks are possible
==2527== 
==2527== For counts of detected and suppressed errors, rerun with: -v
==2527== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

that's the goal of modifying the program.

Answer 2

What your teacher is likely fishing for, is that those assignments you point out only change where the pointers point, they do not make a hard copy of the data. There's two blatant bugs:

• Upon c = b; you get a memory leak as the memory which c pointed at is lost.
• And then when you later free what b points at, you also free what c now points at.

One solution would be to never allocate any memory for c, assign c to point at the same memory as b and then don't free what b points at.

Answer 3

Remove this:

c = (char *) malloc(sizeof(char) * 50);

you don't need to allocate a third buffer, you never use it and in fact overwrite it when doing the swap. Just let c be a temporary char *.

Also, the allocations can be simplified to:

a = malloc(50);
b = malloc(50);

since you don't need to cast, and sizeof (char) is always 1.

For completeness' sake you should also check that the allocations succeed, before relying on it.

Then as pointed out in a comment remove the return, since that makes no sense at all.