Thread: valgrind Conditional jump or move depends on uninitialised value

In this C program

Code:

#include<stdio.h>
#include<string.h>
#include<math.h>
#include<stdlib.h>
#define p 200
#define l 10
char **ab;
int noofpalindrom=0;
//check palindrome//
int palindrome(char *c)
{
 int i,k;
 k=strlen(c);
 for(i=0;i<k;i++)
  {
  if(c[i]!=c[k-i-1]) return 0;
  }
 return 1;
}
//unique palindrome//
int unique(char *a)
{
int i;
for (i=0;i<noofpalindrom;i++)
if(!strcmp(ab[i],a)) return 0;
return 1;
}

//print i length palnindrome  from string b
int f1(int i, char *b, char c)
{
if(i>=0 && b[i]==c )  return 1;
return 0;
}
void f(char *b,int n)
{
 int i,j,k=0,m=0;
 char *d;
 d=(char *)malloc(sizeof(char)*(n+1));
 k=strlen(b);
 for(i=0;i<k-n+1;i++)
  {
   for(j=i;j<i+n;j++)
    {
    d[m]=b[j];
    m++;
    }
   m=0;
   d[n]='\0';
   if(palindrome(d) && unique(d) && f1(i-3,b,'a') && f1(i-2,b,'b') && f1(i-1,b,'a'))
    {
    strcpy(ab[noofpalindrom],d); 
    noofpalindrom++;
    }
  }
  free(d);

//  for(i=0;i<noofpalindrom;i++) printf("%s\n",ab[i]);
}
//thue morse extension//
void thuemorsemorphism(char *c)
{
 int i,k,j,m;
 for(i=1;i<=l;i++) 
  {
  k=strlen(c);
  m=k;
   for(j=0;j<k;j++) 
    {
    if(c[j]=='a') c[m]='b'; 
    if(c[j]=='b') c[m]='a'; 
    m++;
    }
  }
}

int main()
{
 int p1=1,i,length,length1;
 char *a;
 length1=pow(2,l-1);
 length=pow(2,l);
 a=(char *)malloc(sizeof(char)*(length+1));
 a[0]='a';
 a[1]='\0';
 ab=(char **)malloc(sizeof(char *)*length1);
 for(i=0;i<length1;i++) ab[i]=(char *)malloc(sizeof(char)*length1); 
 thuemorsemorphism(a);
 while(p1<p)
 {
 f(a,p1);
 p1=p1+1;
 }
 free(a);
 for(i=0;i<length1;i++) free(ab[i]);
 free(ab);
 printf("\n");
 return 0;
}

after compilation, when I type

[CODE]

Code:

valgrind --tool=memcheck --leak-check=yes --show-reachable=yes --num-callers=20 --track-fds=yes --track-origins=yes ./ms2

it gives

Code:

Conditional jump or move depends on uninitialised value(s)
==2936==    at 0x4C2E0F8: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2936==    by 0x40093C: thuemorsemorphism (ms2.c:66)
==2936==    by 0x400A54: main (ms2.c:88)
==2936==  Uninitialised value was created by a heap allocation
==2936==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2936==    by 0x4009E0: main (ms2.c:83)
==2936== 
==2936== Conditional jump or move depends on uninitialised value(s)
==2936==    at 0x4C2E0F8: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2936==    by 0x4007DA: f (ms2.c:40)
==2936==    by 0x400A67: main (ms2.c:91)
==2936==  Uninitialised value was created by a heap allocation
==2936==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2936==    by 0x4009E0: main (ms2.c:83)

I am not able to resolv this error. It shows error on line

Code:

a=(char *)malloc(sizeof(char)*(length+1));

This program I am not able to run for p=200. But it runs successfully for l=100.

> length1=pow(2,l-1);
> length=pow(2,l);
And what values do you see here?
Considering that running into numeric overflow in such a massive way.

It is great that you thought about using a tool like valgrind, but before you do so, you should format your code such that it is readable. Presently, your indentation is terribly inconsistent, and when you do have indentation, it is only one space, not enough for visual impact! For example, without changing the non-whitespace code, I might format your code like this:

Code:

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

#define p 200
#define l 10

char **ab;
int noofpalindrom = 0;

//check palindrome//
int palindrome(char *c)
{
    int i, k;
    k = strlen(c);
    for (i = 0; i < k; i++)
    {
        if (c[i] != c[k - i - 1])
            return 0;
    }
    return 1;
}

//unique palindrome//
int unique(char *a)
{
    int i;
    for (i = 0; i < noofpalindrom; i++)
        if (!strcmp(ab[i], a))
            return 0;
    return 1;
}

//print i length palnindrome  from string b
int f1(int i, char *b, char c)
{
    if (i >= 0 && b[i] == c)
        return 1;
    return 0;
}

void f(char *b, int n)
{
    int i, j, k = 0, m = 0;
    char *d;
    d = (char *)malloc(sizeof(char) * (n + 1));
    k = strlen(b);
    for (i = 0; i < k - n + 1; i++)
    {
        for (j = i; j < i + n; j++)
        {
            d[m] = b[j];
            m++;
        }
        m = 0;
        d[n] = '\0';
        if (palindrome(d) && unique(d) && f1(i - 3, b, 'a') && f1(i - 2, b, 'b') && f1(i - 1, b, 'a'))
        {
            strcpy(ab[noofpalindrom], d);
            noofpalindrom++;
        }
    }
    free(d);

    //  for(i=0;i<noofpalindrom;i++) printf("%s\n",ab[i]);
}

//thue morse extension//
void thuemorsemorphism(char *c)
{
    int i, k, j ,m;
    for (i = 1; i <= l; i++)
    {
        k = strlen(c);
        m = k;
        for (j = 0; j < k; j++)
        {
            if (c[j] == 'a')
                c[m] = 'b';
            if (c[j] == 'b')
                c[m] = 'a';
            m++;
        }
    }
}

int main()
{
    int p1 = 1, i, length, length1;
    char *a;
    length1 = pow(2, l - 1);
    length = pow(2, l);
    a = (char *)malloc(sizeof(char) * (length + 1));
    a[0] = 'a';
    a[1] = '\0';
    ab = (char **)malloc(sizeof(char *) * length1);
    for (i = 0; i < length1; i++)
        ab[i] = (char *)malloc(sizeof(char) * length1);
    thuemorsemorphism(a);
    while (p1 < p)
    {
        f(a, p1);
        p1 = p1 + 1;
    }
    free(a);
    for (i = 0; i < length1; i++)
        free(ab[i]);
    free(ab);
    printf("\n");
    return 0;
}

But okay, formatting is one thing. You have other things that should be changed before you reach for valgrind:

• You defined two constants as macros, but both have single letter variable names, in lowercase. Macros do not obey the usual rules of scope, so generally you should define them with very descriptive names, and they are typically named with fully uppercase names (especially for constants; sometimes function-style macros will be named similiarly to functions, but again their names will be very descriptive, not single letter, and sometimes they might even have a prefix so as to avoid name collision).
• You declared two global variables, one of which has the poor name of ab. Generally, you should avoid global variables, e.g., by passing variables where needed, because global variables tend to make it more difficult to reason about and maintain your program. If you must have global variables, you must name them with very descriptive names. That is, as a rule of thumb, the larger the scope of a variable, the more descriptive its name must be. noofpalindrom is not too bad, but it seems odd to leave out the "e" at the end of "palindrome" (plus it should probably be plural), and often where names consist of multiple words, either a convention of separating words by underscores would be chosen, or a camel case style would be chosen, e.g., no_of_palindromes or noOfPalindromes. Frankly, this variable doesn't need to be global: out of the six functions in the program, it is only used in two. Likewise ab, which probably should be renamed to palindromes, is only used in three of the six functions. You should declare them locally and pass them around when needed instead.
• Your functions named f and f1 should be renamed with descriptive names. The comment for f1 presents a possible name, yet looking at the code, it doesn't do what the comment says.
• Local variables should be named descriptively too. i as a loop index is a common convention and hence well and good, but it is not clear what is p1 or a, and while length and length1 obviously refer to lengths, exactly what is the difference between length and length1 is not clear from their names.
• It is great that you took care to match malloc calls with free calls, but you should check that malloc did not return a null pointer before using the pointer returned. (This might be what valgrind is referring to, or maybe it is something else.)

Oh, and although this is not something that you need to change, it is something to consider: when calling malloc with sizeof for a non-void pointer, consider making use of the pointer name in the sizeof expression so as to avoid the mistake of using the wrong type. Also, it is unnecessary to cast the return value of malloc. So, this:

Code:

ab = (char **)malloc(sizeof(char *) * length1);

could become:

Code:

ab = malloc(sizeof(*ab) * length1);