Why's _itoa causing my program to crash?

Question

The following code just keeps on crashing when it reaches the part with _itoa, I've tried to implement that function instead and then it got even weirder, it just kept on crashing when I ran it without the debugger but worked fine while working with the debugger.

# include "HNum.h"
# include <stdio.h>
# include <stdlib.h>
# include <string.h>
# include <assert.h>
# define  START_value 30


typedef enum {
    HNUM_OUT_OF_MEMORY = -1,
    HNUM_SUCCESS = 0,
} HNumRetVal;

typedef struct _HNum{
    size_t Size_Memory; 
    char* String;
}HNum;

HNum *HNum_alloc(){
    HNum* first = (HNum*)malloc(sizeof(HNum));
    if(first==NULL){
        return NULL;
    }
    first->String =(char*)malloc(sizeof(START_value));
    if(first->String==NULL){
        return NULL;
    }
    first->Size_Memory = START_value; // slash zero && and starting from zero index;
    return first;
}

HNumRetVal HNum_setFromInt(HNum *hnum, int nn){
    itoa(nn,hnum->String,10);
}

void main(){
    HNum * nadav ;
    int h = 13428637;
    nadav = HNum_alloc();
    nadav->String="1237823423423434";
    HNum_setFromInt(nadav,h);
    printf("nadav string : %s \n ",nadav->String);
    //printf("w string %s\n",w->String);
    //printf("nadav string %s\n",nadav->String);
    HNum_free(nadav);

}

I've been trying to figure this out for hours and couldn't come up with anything... The IDE I'm using is Visual Studio 2012 express, the crash shows the following: "PROJECT C.exe has stopped working windows can check online for a solution to the program."

Run it with the debugger and latter will tell you where exactly it crashes. Then tell us. — Jabberwocky, May 19 '14 at 13:05
@alk There are two errors in the code. The post you link to covers one of these nicely but doesn't help with the other (`sizeof`) issue. — simonc, May 19 '14 at 13:10
Sure, however the question was why it crashes in `itoa()`. @simonc — alk, May 19 '14 at 13:11

simonc · Answer 1 · 2014-05-19T13:59:18.710

4

first->String =(char*)malloc(sizeof(START_value));

should be

first->String = malloc(START_value);

The current version allocates space for sizeof(int)-1 characters (-1 to leave space for the nul terminator). This is too small to hold your target value so _itoa writes beyond memory allocated for first->String. This results in undefined behaviour; it is quite possible for different runs to fail in different places or debug/release builds to behave differently.

You also need to remove the line

nadav->String="1237823423423434";

which leaks the memory allocated for String in HNum_alloc, replacing it with a pointer to a string literal. This new pointer should be considered to be read-only; you cannot write it it inside _itoa

edited May 19 '14 at 13:59

answered May 19 '14 at 13:05

simonc

41,632
12
85
103

I tried initializing the string with 50 and it'd still crash all the same, and about changing that line of code you mentioned, I suppose that's indeed the correct way to write it, but my code should still work without it, right? – user3652695 May 19 '14 at 13:09
You need to make both of the changes I noted. The code will not work reliably without them. – simonc May 19 '14 at 13:11
It won't let me write it like that "first->String = malloc(START_value);" Says that "Error: a value of type "void *" cannot be assigned to an entity of type "char *" And changing nadav->String="1237823423423434"; to strcpy(nadav->String, "1237823423423434"); causes this program to crash on the strcpy line too – user3652695 May 19 '14 at 13:27
2

Sounds like you're using a C++ compiler on a C project. If you use a C compiler you won't require the cast. You'll need to reinstate the `(char*)` cast if you want to continue using a C++ compiler. Your use of `strcpy` should be fine but is not required. Are you sure you'd fixed the `malloc` line when you tried this? – simonc May 19 '14 at 13:32
@simonc: The cast won't hurt with a C compiler. And I think it will allocate `sizeof(int)` bytes, not `sizeof(int)-1` – Aaron Digulla May 19 '14 at 13:48
3

@AaronDigulla Agreed that the cast is permissible. I was suggesting it wasn't desirable (Most but not all posts in [should I cast the return from malloc](http://stackoverflow.com/q/605845/311966) agree that the cast isn't desirable). I also agree that `sizeof(int)` bytes are allocated. Since this memory will be used for a nul-terminated string, we need to allocate space for the terminator. This leaves `sizeof(int)-1` bytes left for output characters. I'll see if I can reword to make that clearer. – simonc May 19 '14 at 13:55

MrGuy · Answer 2 · 2014-05-19T14:18:59.160

0

Since I'm not allowed to comment:

simonc's answer is correct. If you find the following answer useful, you should mark his answer as the right one:P I tried that code myself and the only thing missing is lets say:

strcpy(nadav->String, "1237823423423434"); INSTEAD OF nadav->String="1237823423423434";

and

first->String = malloc(START_value); INSTEAD OF first->String =(char*)malloc(sizeof(START_value));

Also, maybe you'd have to use _itoa instead of itoa, that's one of the things I had to change in my case anyhow.

If that doesn't work, you should probably consider using a different version of VS.

edited May 19 '14 at 14:18

answered May 19 '14 at 13:44

MrGuy

654
1
5
18

The most important part of the `malloc` change is to allocate the correct number of bytes. `malloc(sizeof(START_value))` will probably give you a 4 byte buffer. This isn't long enough to hold a textual representation of any integer so isn't an optional part of the fix. Which is all a roundabout way of saying that the code should (and probably will) *not* work without both changes. – simonc May 19 '14 at 13:58
Oh right, my bad, I meant to write it without the "sizeof" part. What I meant to say is that "first->String =(char*)malloc(START_value);" would've worked. Or is that the same thing as you wrote? – MrGuy May 19 '14 at 14:03
1

Yes, that's the same as I tried to write. I could obviously have been clearer though :-) – simonc May 19 '14 at 15:03

Why's _itoa causing my program to crash?

2 Answers2