CODEWITHSUNDEEP
cstringpointerssegmentation-faultdynamic-memory-allocation
Arcturus
Arcturus

Reputation: 252

Why is heap corruption happening?

I've been working on some practice problems for school, and in one of them, an inputted integer has to be written to a dynamically allocated string. The code does it's job fine until it gets to freeing the allocated memory, where heap corruption strikes.

Can someone please explain why it's happening and what I'm doing wrong? 

int main() {

  char *string = NULL;
  char **string2 = &string;

  Conversion(string2);
  printf("Entered number converted to string: %s", string);
  free(string);

  return 0;
}

int Conversion(char **string) {

    int num = 0, temp = 0, dcount = 0;
    printf("Enter number: ");
    scanf(" %d", &num);

    temp = num;

    while (temp != 0) {
      temp /= 10;
      dcount++;
    }

    *string = (char*)malloc(dcount*sizeof(char));
    if (*string == NULL) {
      printf("Error during memory allocation!");
      return -1;
    }

    sprintf(*string, "%d", num);

    return 0;
}

Upvotes: 1

Views: 319

Answers (2)

Vlad from Moscow
Vlad from Moscow

Reputation: 310980

The function has several drawbacks.

For starters you do not take into account the terminating zero '\0' for the allocated string.

Secondly, the user can enter 0 as a number. In this case the value of dcount will be incorrect because the loop 

while (temp != 0) {
  temp /= 10;
  dcount++;
}

will be skipped.

Thirdly, the function will not correctly process negative numbers and the user is allowed to enter a negative number.

This declaration

char **string2 = &string;

is redundant. You could call the function the following way

Conversion( &string );

It is not the function that should ask the user to enter a number. The function should do only one task: convert an integer number to a string.

And one more function design drawback.

From the function declaration

int Conversion(char **string);

(that should precede the function main) it is unclear whether the user should provide memory for the string or it is the function that allocates memory for the string.

A better interface of the function can look the following way as it is shown in the demonstrative program below.

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

char * Conversion( int x )
{
    const int Base = 10;

    size_t digits = x < 0 ? 1 : 0;

    int tmp = x;

    do
    {
        ++digits;
    } while ( tmp /= Base );

    char *s = malloc( digits + sizeof( ( char )'\0' ) );

    if ( s != NULL ) sprintf( s, "%d", x );

    return s;
}

int main( void )
{
    int num = 0;

    printf( "Enter a number: " );
    scanf( "%d", &num );

    char *s = Conversion( num );

    if ( s != NULL )
    {
        printf( "The entered number converted to string: %s\n", s );
    }
    else
    {
        puts( "A conversion error ocured." );
    }

    free( s );
}

Its output might look the following way

Enter a number: -12
The entered number converted to string: -12

Take into account that according to the C Standard the function main without parameters shall be declared like

int main( void )

Upvotes: 2

John Kugelman
John Kugelman

Reputation: 361615

You need to allocate an extra character to account for the \0 terminator.

*string = (char*)malloc((dcount + 1) * sizeof(char));

Also dcount is incorrect if num is 0 or negative.

Other comments:

  • You could use snprintf() to calculate the needed buffer size. It would save you all the heavy lifting of counting the digits in num, plus it'll handle all the edge cases correctly.

    int dcount = snprintf(NULL, 0, "%d", num);

  • You should avoid casting malloc's return value. 

    *string = malloc((dcount + 1) * sizeof(char));

  • sizeof(char) is defined to be 1. I would omit the multiplication

    *string = malloc(dcount + 1);

    However, if you do leave it, avoid hard coding the item type.

    *string = malloc((dcount + 1) * sizeof(**string));

    If you changed your strings from char* to char16_t*/char32_t*/wchar_t* this would automatically adjust, whereas sizeof(char) would compile without error despite the type mismatch.

Upvotes: 5

Related Questions