8

Here is my code: I am trying to get the info of an struct and deep copy the info. But, valgrind shows that "invalid read". I know that is I read the memory that is released. I don't know why; is anyone able to figure it out for me?

Code

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

struct student
{
    int id;
    char *name;
    int age;
};

void get_info(struct student *dest, struct student *src)
{
    memcpy(dest,src,sizeof(struct student));
    dest->name = strdup(src->name);
}

int main()
{
    struct student foo;
    foo.id = 1001;
    foo.name = strdup("kevin");
    foo.age = 18;

    struct student bar;
    get_info(&bar, &foo);

    puts(bar.name);

    free(foo.name);
    free(bar.name);

    return 0;
}

Valgrind report

valgrind --tool=memcheck --leak-check=full ./test
==2130== Memcheck, a memory error detector
==2130== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==2130== Using Valgrind-3.9.0 and LibVEX; rerun with -h for copyright info
==2130== Command: ./test
==2130== 
==2130== Invalid read of size 4
==2130==    at 0x40B083B: ??? (in /lib/tls/i686/cmov/libc-2.11.1.so)
==2130==    by 0x40B04A4: strdup (in /lib/tls/i686/cmov/libc-2.11.1.so)
==2130==    by 0x80484B1: get_info (test.c:15)
==2130==    by 0x80484F8: main (test.c:26)
==2130==  Address 0x419902c is 4 bytes inside a block of size 6 alloc'd
==2130==    at 0x4026775: malloc (vg_replace_malloc.c:291)
==2130==    by 0x40B04AF: strdup (in /lib/tls/i686/cmov/libc-2.11.1.so)
==2130==    by 0x80484D8: main (test.c:22)
==2130== 
==2130== Invalid read of size 4
==2130==    at 0x40B083B: ??? (in /lib/tls/i686/cmov/libc-2.11.1.so)
==2130==    by 0x409ACE4: puts (in /lib/tls/i686/cmov/libc-2.11.1.so)
==2130==    by 0x8048504: main (test.c:28)
==2130==  Address 0x4199064 is 4 bytes inside a block of size 6 alloc'd
==2130==    at 0x4026775: malloc (vg_replace_malloc.c:291)
==2130==    by 0x40B04AF: strdup (in /lib/tls/i686/cmov/libc-2.11.1.so)
==2130==    by 0x80484B1: get_info (test.c:15)
==2130==    by 0x80484F8: main (test.c:26)
==2130== 
kevin
==2130== 
==2130== HEAP SUMMARY:
==2130==     in use at exit: 0 bytes in 0 blocks
==2130==   total heap usage: 2 allocs, 2 frees, 12 bytes allocated
==2130== 
==2130== All heap blocks were freed -- no leaks are possible
==2130== 
==2130== For counts of detected and suppressed errors, rerun with: -v
==2130== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 11 from 6)
Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
kevin
  • 765
  • 1
  • 6
  • 16
  • @ElliottFrisch Interestingly valgrind doesn't report any errors for me. Maybe undefined behavior? –  Dec 28 '13 at 04:19
  • @remyabel [Yes](http://stackoverflow.com/a/2693681/2970947). – Elliott Frisch Dec 28 '13 at 04:22
  • hi, thanks for your guys help. my environment: ubuntu 10.04 Valgrind-3.9.0 and compiled with gcc 4.4.3. this Invalid read info always displays. – kevin Dec 28 '13 at 04:24
  • The result from strdup should be `free`able, unless someone has rolled their own strdup. I have to admit I don't see UB either at the moment. – tabstop Dec 28 '13 at 04:26
  • I can't reproduce the Invalid read(s) with or without the free(s). – Elliott Frisch Dec 28 '13 at 04:29
  • @tabstop the strdup is of the c library string.h. – kevin Dec 28 '13 at 04:33
  • To be fair, you're using a pretty old version of gcc on an old version of Ubuntu. The packages/libraries could be bugged. –  Dec 28 '13 at 04:34
  • @remyabel it's so weird. i think there is nothing new feature i am using involved for the new gcc. but if all of you cannot reproduce it, it should be as you say, the gcc is too old. – kevin Dec 28 '13 at 04:37
  • @kevin4fly I suppose you should check, assuming you simplified your real code to this example, that you didn't simplify away the problem.... – tabstop Dec 28 '13 at 05:21
  • Note that the `memcpy()` won't be any faster than `*dest = *src;`. – Jonathan Leffler Dec 28 '13 at 06:44

2 Answers2

7

I think this is actually an error report from valgrind that you should suppress. It is ultimately a bug in the C library on your system.

The complaint is that code called from strdup() is reading 4 bytes at an offset of 4 bytes into a block of 6 bytes that were allocated by malloc(). Given that "kevin" occupies 6 bytes, then I believe a memcpy() variant has been employed by strdup() and has been caught in the act of reading 4 bytes at a time through the data. While it is probably actually safe, technically, valgrind is correct to complain. However, you can't do anything about that — your code is innocent, and the system library is guilty. That's the sort of thing suppressions are for!

+---+---+---+---+---+---+...+...+
| k | e | v | i | n | \0| ? | ? |
+---+---+---+---+---+---+...+...+

The fast copy is using the fact that the malloc()'d data is 4-byte (more likely, 8-byte) aligned. It copies the 4 bytes 'k', 'e', 'v', and 'i' in one operation; it then copies the other two bytes of the string ('n', '\0') plus the two bytes that are not technically part of the allocated space in a second operation. The minimum actual allocation is probably 8 bytes on a 32-bit system; it tends to be 16-bytes on 64-bit machines. That means that the extra two bytes are part of the space reserved for the memory allocation, but valgrind is correct to report that the code is copying outside the allocated space.

Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
  • wow, totally make sense. 6 bytes alloced, memcpy copied 8 bytes actually(2 times and 4 bytes each time), that is complained by Valgrind since i "read" the byte beyond what i have alloced. Appreciated! BTW, when the object(struct) is able to assign to other object in pure C? or originally suppored by design? i don't remember it clearly. – kevin Dec 28 '13 at 07:42
  • Structure assignment was added soon after the first edition of K&R was published in 1978; by the time I started working in C in 1983, the C compiler we used already had structure assignment (but not all C compilers did). By the time the C standard was released (1989, 1990), most compilers supported structure assignment (but I worked on a code base that had remnants of the old world in it, with a `STRUCTASG` macro still in use in a few places — much to my exasperation). If `memcpy()` is faster than structure assignment, the compiler writers made a huge mistake somewhere. – Jonathan Leffler Dec 28 '13 at 07:47
1

This looks like an optimization in strdup() and puts(), they read their input in chunks of four bytes instead of one byte at a time, however they are careful not to write beyond the end. This is safe as long as the four byte addresses are aligned: Properly aligned reads can never trigger a hardware exception, and since these two functions do not write beyond the end of the string, their operation is safe, even though it is illegal from a language point of view. And you can be certain that these four byte addresses will be properly aligned because it's an optimization, unaligned reading would bring the code to crawl.

Valgrind checks on the language level of correctness, not on the physical level, hence the error report. So I agree with Jonathan Leffler that you should just suppress this "error", which, looking at the comments, seems to be automatically done by more recent versions of valgrind.

cmaster - reinstate monica
  • 38,891
  • 9
  • 62
  • 106