Pushing a struct to an array which contains an array itself

Question

I was reading this article, which gives an example of a contract which won't work. Though they have fixed it in the article, I wanted to know why it doesn't work. I have copied a simplified version below:

pragma solidity ^0.4.18;

contract StructArrayInitWrong {
  struct Room {
    address[] players;       
  }  
  Room[] rooms;

  function createRoom() public {
    address[] adr;
    adr.push(msg.sender);
    Room memory room = Room(adr);   
    rooms.push(room);
  }

  function getRoomsLength() view returns (uint) {
    return rooms.length;
  }
}

The strange thing is rooms array length increases by two each time createRoom is called in this contract. Can you explain this behaviour? Thanks.

Answer 1

The problem is with this line

address[] adr;

There is a "warning" about uninitialized storage. There's more going on here than a gentle warning might suggest. I really don't understand why this isn't a hard error so the developer doesn't accept it.

Owing to the storage being uninitialized, the compiler didn't know where to put the dynamic array adr[] so it put it in the first slot. You read that right. It's stomping on rooms[]. Ouch!

Since the occupant of that slot also happens to be a dynamic array, it too uses the first word to describe the array length. Thus, we get the strange observed behavior ... push on to one array, then on to the other, and poof! - both arrays have length 2.

This is not the only case where storage declared inside functions instead of in the usual place (outside) causes potentially catastrophic data overwrites. See here for another example: Solc Compiler oversight? Innappropriate mapping declaration overwrites storage

Considering that smart contracts are supposed to be clear and free of defects, I'm not a big fan of this sort of unintended result. This, and reference variables add up to a little too much voodoo for my taste.

Maybe someone else will chime in with better heuristics. I might suggest forming two habits

• Always layout state variables in the same place, near the top, outside functions to avoid this kind of unexpected overwrite.
• Never adjust/set a variable that was previously read from indexed storage because it might unexpectedly overwrite storage. See here: http://vessenes.com/solidity-frustrations-references-and-mapping/

Here's the code with one change and working as expected:

pragma solidity ^0.4.18;

contract StructArrayInitWrong {

  struct Room {
    address[] players;       
  }  
  Room[] rooms;
  address[] adr; // <=== if we're going to store this, then let's store this.

  function createRoom() public {
                                    // <=== note gaping hole
    adr.push(msg.sender);
    Room memory room = Room(adr);   
    rooms.push(room);
  }

  function getRoomsLength() public view returns (uint) {
    return rooms.length;
  }
}

Hope it helps.