3

I got the following code:

/* main.c */

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

int main (){
  int i;
  char *msg = "This is a simple and small message";
  int len = strlen (msg);
  char *new_msg = (char *) malloc (len);
  for (i = 0; i < len; i++)
    new_msg[i] = 'A';
  printf ("%s\n", new_msg);
  free (new_msg);
  return 0;
}

I compiled it and then run it using valgrind with this command:

valgrind --leak-check=full --show-reachable=yes ./main

I got the this output:

==8286== Memcheck, a memory error detector
==8286== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==8286== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==8286== Command: ./main
==8286== 
==8286== Invalid read of size 1
==8286==    at 0x4C2C1B4: strlen (vg_replace_strmem.c:412)
==8286==    by 0x4EA09FB: puts (ioputs.c:36)
==8286==    by 0x400636: main (main.c:12)
==8286==  Address 0x51de062 is 0 bytes after a block of size 34 alloc'd
==8286==    at 0x4C28C20: malloc (vg_replace_malloc.c:296)
==8286==    by 0x400601: main (main.c:9)
==8286== 
AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
==8286== 
==8286== HEAP SUMMARY:
==8286==     in use at exit: 0 bytes in 0 blocks
==8286==   total heap usage: 1 allocs, 1 frees, 34 bytes allocated
==8286== 
==8286== All heap blocks were freed -- no leaks are possible
==8286== 
==8286== For counts of detected and suppressed errors, rerun with: -v
==8286== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

I see that all the allocated memory was released, but I still get an error that I don't understand.

Appreciate the help.

mik mik
  • 139
  • 1
  • 5

3 Answers3

10

This is a pretty straightforward error: there is an invalid read of new_msg because the null terminator is not there.

You have allocated the number of chars equal to the length of the original string, so currently there's no space to fit '\0' without making undefined behavior. Change your code as follows to fix the problem:

char *new_msg = malloc (len+1);
for (i = 0; i < len; i++)
    new_msg[i] = 'A';
new_msg[len] = '\0';
Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • First of all, thank you for the explanation. Now it works. Secondly, I saw that you removed the cast of the malloc. Why is that? I run it with and without the cast and still got the same output (even on valgrind). Is there any difference? – mik mik Nov 03 '15 at 09:07
  • @mikmik [Here is a good Q&A on casting `malloc` results](http://stackoverflow.com/q/605845/335858). The idea behind not casting is that you have specified the type already, so you are repeating the same piece of code again. A cast could also hide subtle errors. – Sergey Kalinichenko Nov 03 '15 at 09:12
1

There are a number of things to be changes in your code.

1) len should be of size_t not int, as strlen() returns of type size_t

2) (char *) malloc (len); drop the cast. This isn't an error, although there are reasons one should not cast.

3) new_msg is not NULL terminated \0. This is the reason the error is occurring.

Haris
  • 12,120
  • 6
  • 43
  • 70
  • 1
    Numbers 1 and 2 are not reason for this issue. It's OK to mention them, but please make the distinction clear. – user694733 Nov 03 '15 at 09:05
  • @Haris The cast is not an error but very useful information on the one hand and allows to prevent iincorrect assignment. on the other hand – Vlad from Moscow Nov 03 '15 at 09:06
0

you use strlen() to get length, but not contain the '\0'.
so when you malloc a new array, you should use len + 1, and set new_msg[len] is '\0'.

He Yuntao
  • 86
  • 11