valgrind conditional jump or move depends on uninitialized value

Question

I have the code below that fixes the IP addresses to 15 digit by padding 0 as prefix.

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

char *function(char *newhost){
     char *IPaddr;
     IPaddr = (char *)calloc(16, sizeof(char));

    size_t i=0 , j= 0;
    for(i=0, j=0; j<15; i++, j++){
        if((newhost[strlen(newhost)-(i+1)] == '.')){   //////////line 11
            if( j == 3 || j == 7 || j == 11){
                IPaddr[14-j] = '.';
            }else if(j<3){
                while(!(j==3)){
                    IPaddr[14-j]='0';
                    j++;
                }
                IPaddr[14-j] = '.';
            }else if(j > 3 && j<7){
                while(!(j==7)){
                    IPaddr[14-j]='0';
                    j++;
                }
                IPaddr[14-j] = '.';
            }else if(j>7 && j<11){
                while(!(j==11)){
                    IPaddr[14-j]='0';
                    j++;
                }
                IPaddr[14-j] = '.';
            }
        }else if(newhost[strlen(newhost)-(i+1)] == '\0'){ ///////////line33
            while(!(j==15)){
                IPaddr[14-j] = '0';
                j++;
               }
        }else{
            IPaddr[14-j] = newhost[strlen(newhost)-(i+1)];
        }
    }
    printf("IPaddr: %s\n", IPaddr);
    return IPaddr;

}


int main(int argc,char *argv[]){  /////////line48
    char host[100] = {'\0'};
    strcpy(host, "10.0.0.2");
    char *new_IP;
    new_IP = function(host);  ////////////line52
    printf("newIP:%s\n",new_IP);
    free(new_IP);

return 0;
}

The code works and compiler outputs neither error nor warning. However, valgrind outputs (valgrind --tool=memcheck --leak-check=yes --track-origins=yes test )

==22544== Conditional jump or move depends on uninitialised value(s)
==22544==    at 0x8048547: function (test.c:11)
==22544==    by 0x804872B: main (test.c:52)
==22544==  Uninitialised value was created by a stack allocation
==22544==    at 0x80486DB: main (test.c:48)
==22544== 
==22544== Conditional jump or move depends on uninitialised value(s)
==22544==    at 0x8048654: function (test.c:33)
==22544==    by 0x804872B: main (test.c:52)
==22544==  Uninitialised value was created by a stack allocation
==22544==    at 0x80486DB: main (test.c:48)

Could anyone tell me how I can fix the code?

Solution

The trouble is that in the function, you are doing:

for (i=0, j=0; j < 15; i++, j++){
    if ((newhost[strlen(newhost)-(i+1)] == '.')){  

As you go through the loop, i becomes larger than the length of the initialized string, but I'd expect that to generate an 'out of bounds' error. However, because the variable in main() is not dynamically allocated, it may well be that valgrind can't help more. I recommend modifying main() to:

int main(int argc, char *argv[])
{
    char *host = malloc(100);
    if (host != 0)
    {
        strcpy(host, "10.0.0.2");
        char *new_IP = function(host);
        printf("newIP:%s\n", new_IP);
        free(host);
        free(new_IP);
    }
    return 0;
}

I expect valgrind to complain about more problems, in particular, out of bounds memory access.

Separately:

• Since strlen(newhost) doesn't change while the function is running, you should compute it once, outside of the loop.
• I'm not sure why you're using the if ((...)) notation. If it is a reflex action to avoid the compiler warning about using assignments in conditions, you're defeating the purpose of warning.

• Would it be easier to parse the string into 4 numbers with sscanf() and then format them using sprintf()?

  char *function(const char *newhost)
  {
      int o1, o2, o3, o4;
      char *result = 0;
      if (sscanf(newhost, "%d.%d.%d.%d", &o1, &o2, &o3, &o4) == 4)
      {
          /* Should check that values are in range 0..255 */
          result = malloc(16);
          if (result != 0)
              sprintf(result, "%.3d.%.3d.%.3d.%.3d", o1, o2, o3, o4);
      }
      return result;
  }
  

Another alternative implementation of your function(), using string copying only:

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

char *function(const char *newhost)
{
    char *result = malloc(16);
    if (result != 0)
    {
        strcpy(result, "000.000.000.000");
        const char *beg = newhost;

        for (int i = 0; i < 4; i++)
        {
            const char *end = strchr(beg, '.');
            if (end == 0)
                end = beg + strlen(beg);
            memcpy(result + (i * 4) + 3 - (end - beg), beg, end - beg);
            beg = end + 1;
        }
    }
    return result;
}

int main(void)
{
    char host[] = "10.0.0.2";
    char *newhost = function(host);
    printf("%s => %s\n", host, newhost);
    free(newhost);
    return 0;
}

This calculates how many digits are in each segment, and then copies it to the right place in the result buffer which has already been filled with zeroes and dots at the right places. It avoids all the magic numbers like 3, 7, 11 that litter the original code.

I'd also recommend a different interface to function, avoiding dynamic memory allocation:

void function(const char *oldhost, char *newhost)

We can debate about void vs int, but the calling function should supply the (known, fixed size) buffer where the output is to be written, so as to avoid having to do dynamic memory allocation. You'd use int if the function does any validation on the string it is given; otherwise, it can be void. The existing code mostly does not report an error if passed an erroneous IP address.

Comments

[Jonathan Leffler]

You could use %.3d or %03d; they both produce leading zeroes.

[Carl Norum]

OK. I've never seen %. used that way before. I'll have to check that out.