Why doesn't my function work inside a loop?

Question

Code:

#include <iostream>

char* intToCharArray(int val)
{
    int len=0;
    bool neg = false;

    if (val < 0)
    {
        val = -val;
        len = ((int)log10(val)) + 2;
        neg = true;
    }
    else
    {
        len = ((int)log10(val)) + 1;
    }

    char* cons = new char[len+1];

    if (neg)
    {
        cons[0] = '-';
        for (int i = 1; i <= len-1; i++)
        {
            cons[len-i] = (val % 10) + '0';
            val /= 10;
        }
    }
    else
    {
        for (int i = 0; i <= len; i++)
        {
            cons[len-i-1] = (val % 10) + '0';
            val /= 10;
        }
    }

    cons[len] = '\0';

    return cons;
}

int main(int arc, char* arv)
{
    int num = 444;
    char* arr = intToCharArray(num);
    int count = 0;
    std::string test = std::string(arr);
    std::string beg = "test";

    std::cout << test << std::endl;

    //changing it like this works
    arr = intToCharArray(num * 6);
    test = std::string(arr);

    std::cout << test << std::endl;

    //but changing it with a loop crashes the program
    for (int i = 0; i < 5; i++)
    {
        arr = intToCharArray(num * i);
        test = std::string(arr);

        std::cout << test << std::endl;
    }

    return 0;
};

Basically, I'm trying to convert an integer into a string. Everything seems to work... unless I run the code in a loop. I have absolutely NO idea what could even possibly be the problem. I've never run into, or even heard of, a problem like this. I'm honestly not even sure what to ask, so Google has been no help either.

Answer 1

Ok theres to be 3 bugs in this code, let's tackle the immediate cause of the crash, the mishandled 0.

log10(0) outputs a giant negative number which then causes the new char[] to throw bad_alloc. To fix this you can change the line in the else clause (of if (val < 0)) to:

len = val == 0 ? 1 : ((int)log10(val)) + 1;

Another more sneaky problem which doesn't cause a crash on your machine (or mine, or the online compiler I used) is writing out of bounds in the array

for (int i = 0; i <= len; i++)

This stopping condition for this loop should be i < len otherwise you get an extra iteration where val is equal to 0 and index is equal to -1 which writes outside of the bounds of the array you allocated (which is UB and may manifest in a number of different ways).

The negative loop while not buggy can be simplified, i <= len - 1 is the same as the more conventional i < len.

Another bug is the memory leak. Each call to intToCharArray new's an array which is never de-allocated anywhere. You could fix this by calling delete arr; after creating a std::string from it, but this is error prone. It would be simpler to create a string in intToCharArray instead.

This is the 'final version':

#include <iostream>
#include <cmath>
#include <string>

std::string intToCharArray(int val)
{
    int len = 0;
    bool neg = false;

    if (val < 0)
    {
        val = -val;
        len = ((int)log10(val)) + 2;
        neg = true;
    }
    else
    {
        len = val == 0 ? 1 : ((int)log10(val)) + 1;
    }
    
    std::string cons; cons.resize(len + 1);

    if (neg)
    {
        cons[0] = '-';
        for (int i = 1; i < len; i++)
        {
            //std::cout << "Writing digit " << char((val % 10) + '0') << " at index " << (len - i) << '\n';
            cons[len - i] = (val % 10) + '0';
            val /= 10;
        }
    }
    else
    {
        for (int i = 0; i < len; i++)
        {
            //std::cout << "Writing digit " << char((val % 10) + '0') << " at index " << (len - i - 1) << '\n';
            cons[len - i - 1] = (val % 10) + '0';
            val /= 10;
        }
    }

    cons[len] = '\0';

    return cons;
}

int main(int arc, char** arv)
{
    int num = 444;
    
    for (int i = 0; i < 5; i++)
    {
        std::string test = intToCharArray(num * i);
        std::cout << test << std::endl;
    }

    for (int i = -1; i > -5; i--)
    {
        std::string test = intToCharArray(num * i);
        std::cout << test << std::endl;
    }

    return 0;
}

With this code I get the following output:

0
444
888
1332
1776
-444
-888
-1332
-1776

Answer 2

You are writing out of bounds here.

Lets assume len is two. Then you allocate 3 bytes.

char* cons = new char[len+1];
for (int i = 0; i <= len; i++)
{
    cons[len-i-1] = (val % 10) + '0';
    val /= 10;
}

Now the loop is:

len(2)-i(0)-1 = cons[1]
len(2)-i(1)-1 = cons[0]
len(2)-i(2)-1 = cons[-1]

It should be:

for (int i = 0; i < len; i++)