Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Please review the following code:

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

int main(void) 
{
    char *some_text = "THIS IS SOME ARBITRARY TEXT FOR TESTING THE STRLEN AND COPY FUNCTIONS";

    char *test = malloc(strlen(some_text) + 1);
    strncpy(test, some_text, strlen(some_text));
    strcat(test, "\0");

    printf("%s\n", test);
    if (test != NULL) free (test);
}
  1. create a char* some_text with constant string;

  2. create a char*test with and allocate memory the size of some_text

  3. copy the content of some_text to test

  4. concatenate the termination character to test

  5. print out the test

  6. free the test pointer

If I run this with valgrind, it says:

==31176== Conditional jump or move depends on uninitialised value(s)
==31176==    at 0x4008667: __GI_strlen (mc_replace_strmem.c:404)
==31176==    by 0x861DD4: puts (in /lib/libc-2.12.so)
==31176==    by 0x80484DF: main (strlen_and_copy.c:13)

The line mentioned in valgrind output strlen_and_copy.c:13 is below:

printf("%s\n", test);

Is it avoidable or should it be ignored? What is wrong with my code above? Someone please explain.

share|improve this question
    
Is that Valgrind's cryptic way of complaining that you didn't check whether malloc() returned NULL? –  200_success Apr 8 at 8:15
add comment

2 Answers 2

up vote 5 down vote accepted

Some general remarks to your code:

  1. You should store the string length as a variable rather than calling it multiple times.
  2. If you do strncpy(test, some_text, strlen(some_text) + 1) you can get rid of the strcat as strncpy will padd the remainder with \0 if src is less than n characters.
  3. You don't need to test for NULL before free
    • a) because you'd get a segfault anyway before that line of code if it were NULL
    • b) because the standard guarantees that it is safe to call free(NULL)
share|improve this answer
    
Thaks @chrisWue, Actually why this output is coming in valgrind ==31176== at 0x4008667: __GI_strlen (mc_replace_strmem.c:404) ? Still it is availeble –  BlueBird Apr 8 at 8:32
    
Thaks @chrisWue, Ok. your 2nd point made it success. –  BlueBird Apr 8 at 8:38
add comment

First string you pass to strcat is not null-terminated, which is incorrect (it takes two null-terminated byte strings). Actually it is null-terminated, but only because malloc returns pointer to zeroed memory on your system. Anyway, after call to strncpy you have a string with last byte uninitialized.

This code

strcat(test, "\0");

does nothing ("\0" and "" are equal).

To make it correct you can write

strcpy(test, some_text);
// without strcat

or

strncpy(test, some_text, strlen(some_text) + 1)

or

strncpy(test, some_text, strlen(some_text));
test[strlen(some_text)] = 0;
share|improve this answer
4  
Good observation that strcat() cannot be used to add a NUL terminator to a string that doesn't already have one. Also a good observation that test is NUL terminated more by luck than by design. However, @ChrisWue's strncpy(test, some_text, strlen(some_text) + 1) is a better solution. –  200_success Apr 8 at 9:12
    
Thanks, I'll add it to my answer –  Maciej Chałapuk Apr 8 at 9:15
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.