Solc Compiler oversight? Innappropriate mapping declaration overwrites storage

Question

I have written a contract that has the mapping storage variable A & B. These variables get initialized with some values in constructor. A method getBalance returns the balance from mapping A & B.

In the method I declared another mapping - thinking that it would give a compilation error (since mapping allowed for storage only). Instead it lead to a strange behavior - appears that the new local mapping overwrites mappingA.

Result without local mapping (100, 200) as expected Result with local mapping (500, 200) <<< Why?

/** Tested in Truffle/TestRPC **/

contract Mapping {

  mapping(address => uint)  balancesA;
  mapping(address => uint)  balancesB;

  function Mapping() {
    // constructor
    balancesA[msg.sender] = 100;
    balancesB[msg.sender] = 200;
  }

  function getBalance(address addr) returns(uint, uint) {

    // If these 2 lines are commented then behavior is as expected
    mapping(address => uint)  balancers;
    balancers[msg.sender] = 500;

    return (balancesA[addr], balancesB[addr]);

  }
}

I can confirm that. The mapping in the function seems to collide with the first mapping that was declared properly. — Rob Hitchens, Mar 01 '17 at 13:20
Have you tried running it on testnet (or private network)? Sometimes TestRPC behaves differently then "real" network - maybe thats the case. — radmen, Mar 01 '17 at 13:38
Another interesting one .... if I change the order of declaration...declare balancesB before balancesA ..... then it collides with balancesB so the first 1 is getting overwritten i.e., result is (100 500) — ACloudFan, Mar 01 '17 at 18:35
@ACloudFan It looks like it's bug report time. Do you want to do the honors? — Rob Hitchens, Mar 02 '17 at 04:46
Bug aside, the return parameters aren't being used correctly. Should be returns (uint, uint) rather than being named, or balanceA = balancesA[addr]; etc — o0ragman0o, Mar 03 '17 at 00:50

Rob Hitchens · Answer 1 · 2017-03-06T17:25:05.300

5

Opened an issue here, github, ethereum, solidity, issue 1731: https://github.com/ethereum/solidity/issues/1731

If I understand their process correctly, original issue 1731 is closed by issue 1737 which is now merged. Possibly the compiler is going to emit an error for this sort of thing in the future.

Both thumbs up for the devs!

https://github.com/ethereum/solidity/pull/1737

edited Mar 06 '17 at 17:25

answered Mar 03 '17 at 04:11

Rob Hitchens

55,151
11
89
145

BTCETHswap · Answer 2 · 2017-03-03T18:56:01.990

2

I'm afraid this is not a bug. I very much agree that it's a problem and presents a huge trap, though.

Please see the documentation here: https://solidity.readthedocs.io/en/v0.4.9/types.html#data-location

As mentioned in max taldykin's answer: By declaring the local variable balancers inside the function you're merely creating a new reference with a different name that points to a mapping in storage. I guess it just links to the first one it finds declared on the global level.

Declaring the local variable as memory would prevent the overwrite.

mapping(address => uint) memory balancers;

To be clear: this problem only applies to non-primitives (structs, mappings, and arrays) when declared locally inside functions as their default location is storage as explained in the doc linked above!

edited Mar 03 '17 at 18:56

answered Mar 03 '17 at 18:47

BTCETHswap

376
2
6

1

the bug is that compiler does not warn about using uninitialized variable. – max taldykin Mar 03 '17 at 19:31
Thanks. Interesting, and scary. Maybe not a "bug" but not helpful or wanted in an unforgiving environment. Sort of like this: http://vessenes.com/solidity-frustrations-references-and-mapping/ reference vars and weird mapping overlaps; I do not like them Sam I Am. – Rob Hitchens Mar 04 '17 at 06:17
@maxtaldykin Seems like the compile folks agree with you. They've added issue 1737 (1731) to the issue I reported and they seem to be working on it. https://github.com/ethereum/solidity/pull/1737 – Rob Hitchens Mar 04 '17 at 06:22

score 1 · Answer 3 · answered Mar 03 '17 at 18:03

Aside from that it is a bug in solidity (as it should notice that uninitialized variable is used), it worth mentioning that the code is not correct also.

The problem is that mapping(address => uint) balancers in function body does not allocate new mapping. It just introduces variable balancers which can reference some mapping (allocated elsewhere).

In the example balancers is not initialized and coincidentially references balancesA. Hence the unexpected behavior.

From Solidity docs:

Mappings are only allowed for state variables (or as storage reference types in internal functions).

Solc Compiler oversight? Innappropriate mapping declaration overwrites storage

3 Answers3

Linked