C++ strcat creating infinite loop

Question

Im trying to create an array and each object in the array should have the name Model(i) where is is index, Im doing this so they will have the names indexed in descending order Model5, Model4 ... Im trying to do that using char[] but for some reason in my code the use of strcat inside of for loop make me get stuck on an infinite loop, the second point if someone could help is convert the index in a way that I could concatenate with the name and give to the constructor.

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

using namespace std;

class CARRO {
    public:
        CARRO() {};
        CARRO(char *modelo, unsigned ano);
        char* getModelo();
        unsigned getAno();
    private:
        char modelo[100];
        unsigned ano;
};

void swap(int *p, int *q);
int partition(int *v, int start, int end);
int randomizedPartition(int *v, int start, int end);
void qsHelper(int *v, int start, int end);
void quickSort(int *v, int len);
void printList(CARRO *carros, unsigned len);

int main(int argc, char const *argv[]) {

    CARRO carros[5];
    unsigned len = sizeof(carros)/sizeof(CARRO);

    for (int i = 0; i < len; ++i) {
        char modelo[] = "Modelo";
        char id[] = "I";
        strcat(modelo, id);
        unsigned ano = 1000 * (i+1);
        carros[i] = CARRO(modelo, ano);
        cout << carros[i].getModelo() << endl;
    }

    //printList(carros, len);

    return 0;
}

CARRO::CARRO(char *modelo, unsigned ano) {
    strcpy(this->modelo, modelo);
    this->ano = ano;
}

If I remove the line :

strcat(modelo, id);

The loop works fine. I just cant understand why strcat is somehow generating an infinite loop. the output is this: (with the line strcat)

ModeloI
ModeloI
ModeloI
ModeloI
ModeloI
ModeloI
ModeloI
ModeloI
ModeloI
ModeloI
^CModeloI

If you're using C++, you should use `std::string` and the associated methods instead of C's `strcat` with character arrays — Govind Parmar, Nov 16 '16 at 00:16
`modelo` is only created large enough to hold the word you initialized it with. If you make its contents longer with `std::strcat` you are writing outside the bounds of the array causing undefined behavior. — Galik, Nov 16 '16 at 00:16
Possible duplicate of [I just can't figure out strcat](http://stackoverflow.com/questions/4707900/i-just-cant-figure-out-strcat) — Ken Y-N, Nov 16 '16 at 00:17
Don't you get warnings on `char modelo[] = "literal"`? I think that you enter anything-can-happen area with your code. Check this: http://stackoverflow.com/questions/8356223/assign-a-string-literal-to-a-char — luk32, Nov 16 '16 at 00:18
@GovindParmar I think I will be ending doing that, but is there something that could fix this bug ? Or some explanation about why strcat is causing that ? Like the loop sentinel is well defined and nothing is changing the value of i inside of the loop. Idk why that is happening :( — Pj-, Nov 16 '16 at 00:18
@luk32 Because `modelo[]` is an array the string literal is copied in so it's perfectly legal to have non-const array. — Galik, Nov 16 '16 at 00:20
@luk32 Thanks I just realized that, when I initialize a char[] with a literal it have fixed size, so copying that is causing the problem. Is just weird how that is causing an infinite loop, do you how could I fix that ? And and the conversion from the index to char and adding at the end of the name ? — Pj-, Nov 16 '16 at 00:23
@Galik You are absolutely right. It is modifiable, but the size is fixed. Well at least I was partially right. `strcat` on it with a non-empty string is guaranteed UB, as you have pointed out. — luk32, Nov 16 '16 at 01:05
Something like `char modelo[10] = "Modelo"` would be just fine in this case. But as @GovindParmar said in the very first comment, you really ought to be using `std::string` unless you have a specific reason to work on the stack. — paddy, Nov 16 '16 at 01:08
Probably the compiler put the loop variable right after the character array, and when writing beyond the bounds, the `\0` terminating the string overwrote the first byte of that variable. On a little-endian architecture like x86, that would overwrite the least significant byte of that variable, which given that the value of that variable never exceeds 255, would reset that variable to 0. Due to that reset it never reaches the value 5 required to end the loop. Note that all this is speculation. Your code has undefined behaviour and *anything* may happen. — celtschk, Nov 16 '16 at 01:11

score 2 · Answer 1 · answered Nov 16 '16 at 04:27

The loop works fine. I just cant understand why strcat is somehow generating an infinite loop. the output is this: (with the line strcat)

The loop doesn't work fine! You are trashing memory. Let's make some boxes to represent the stack as might be seen by your program (let's assume the loop has just gone around to 1):

+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+
| char modelo[7]                          | int i                 | unsigned len          |
+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+
| 'M' | 'o' | 'd' | 'e' | 'l' | 'o' | 0   | 1   | 0   | 0   | 0   | 5   | 0   | 0   | 0   |
+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+

So when you do this:

strcat( modelo, id );

You end up with a buffer overrun by one byte. In my particular example, this writes over the first byte of the variable i, thus causing your loop to continue indefinitely:

+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+
| char modelo[7]                          | int i                 |
+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+
| 'M' | 'o' | 'd' | 'e' | 'l' | 'o' | 'I' | 0   | 0   | 0   | 0   |
+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+
                                           ^^^^^
                                           Nul-terminator written

Of course, I have laid out your stack like this purely as an example. Your compiler might not keep the stack together like this. There might be extra padding after your array and it might just happen to "work". The variable i might be held in a register and never in memory, or the compiler might have unrolled the loop completely. Your architecture might be big-endian (instead of little-endian as in my example).

The point is, the resulting behaviour is completely undefined. We cannot look at this code and say what will happen, even if you get consistent results on your machine.

So to fix this, you can simply make modelo large enough to store the string "ModeloI" including the terminator, which means making it large enough to store 8 bytes instead of 7:

char modelo[8] = "Modelo";

Then you'll have defined behaviour, regardless of whether the stack is laid out as below or any other way:

+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+
| char modelo[8]                                | int i                 |
+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+
| 'M' | 'o' | 'd' | 'e' | 'l' | 'o' | 'I' | 0   | 1   | 0   | 0   | 0   |
+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+

Thank you so much for the explanation @paddy , I've decide to use the std::string so the problem is now fixed! But thanks a lot, now i will be aware when using char[]. — Pj-, Nov 17 '16 at 22:21

C++ strcat creating infinite loop

1 Answers1