Why is heap corruption happening?

Question

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;
}

errors: 1) The sub function: `Conversion()` is missing a prototype, so the compiler will assume the types for the parameter and the returned type, which may (or may not) be correct. 2) The actual sub function returns a 'int' indicating the fail/success of the call to `malloc()` but the call in `main()` fails to check that returned value, 3) when `malloc()` fails, the `main()` function still tries to pass that NULL pointer to `printf()` — user3629249, Jun 07 '19 at 22:30

John Kugelman · Accepted Answer · 2019-06-07T14:17:32.843

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.

Doh! UI knew it was gonna be something stupidly obvious, like always. Thank you for your time, and especially for all the extra comments. snprintf() will be very useful in the future. That's also an interesting read on the casting of malloc, as we were told in class that we need to cast it ourselves because it returns a void pointer. — Arcturus, Jun 07 '19 at 14:40

Vlad from Moscow · Answer 2 · 2019-06-07T14:36:17.687

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 )

It's just a snippet of code with the parts needed to present the issue I was having :) I declare everything AI use in a custom header. But thank you for your answer. Between yours and the one above, I learned a bunch, much more than I was expecting to take away from my question! — Arcturus, Jun 07 '19 at 17:13

Why is heap corruption happening?

2 Answers2