needless storage clear when modifying a short dynamically sized byte array

[cdetrio]

This bug affects Solidity v0.3.6 (at least, didn't test versions prior), and the latest nightly v0.4.3-nightly.2016.9.30.

Take this example contract:

pragma solidity ^0.4.0;
contract c {
    string test;
    function setString(string _test) {
        test = _test;
    }
}

Then pass a short string (i.e. one that fits in a single 32-byte storage slot) to the setString() method. For example, passing "example string":

We can see above that on the first execution of setString(), only one SSTORE occurs.

Now call setString() a second time, for example with "example string 2":

We can see that on the second execution, two SSTOREs occurred. The first SSTORE set the 0x0 slot to "example string 2" (0x6578616d...), and the second SSTORE cleared the 0x290de... slot (i.e. set the slot to 0). But since the 0x290de... slot was already empty, the second SSTORE was a needless operation.

This needless storage clear originates at libsolidity/codegen/ArrayUtils.cpp#L99-L100, since if the target storage size (i.e the value of convertLengthToSize(_targetType)) is 1, after adding it to target_data_pos, target_data_pos and target_data_end will span two storage slots. After the new string is written to the first storage slot (overwriting the previously-existing value at that slot), the second slot is then cleared in clearStorageLoop(*targetBaseType). On the first execution, the bug did not occur because the target storage size was 0, so target_data_pos and target_data_end are equal (and thus span only a single storage slot).

[pirapira]

I am wondering why this bug happens especially for "target storage size == 1". Following the description, I don't see any reason why the same thing does not happen for "target storage size == 2". In that case, target_data_pos and target_data_end would span three storage slots, which is bigger than two.

[cdetrio]

Its a bug for target_storage_size == 1 because in that case, the storage length and the storage value both fit in the same slot. So to overwrite it, there only needs to be one SSTORE operation.

For target_storage_size == 2, storing the value requires three storage slots: the first slot stores the length of the value, and the second and third slots store the value. So to overwrite it, there should be three SSTORE operations. More accurately it is target_ref and target_data_end that span three storage slots; target_ref is the first slot, target_data_pos is sha3(target_ref) and so is the second slot, target_data_pos+1 is the third slot, and target_data_pos+2 == target_data_end.

Walking through the case of target_storage_size == 2 (two calls to setString("asdf asdf asdf asdf asdf asdf asdf") in the example contract):

The first SSTORE sets the 0x00 location (target_ref) to the target length (2*length+1). The second SSTORE is done at target_data_pos (this line), 0x290de...ef3e563. Then target_data_pos is incremented, resulting in 0x290de...ef3e564. It jumps back to copyLoopStart, and the third SSTORE is done at 0x290de...ef3e564. After being incremented again, 0x290de...ef3e565 is now equal to target_data_end.

When target_data_pos and target_data_end are equal, then the GT (greater than) check in clearStorageLoop() is false, which results in jumping over the storage clear operation StorageItem().setToZero().

By treating the target_storage_size == 1 as a special case, setting target_size = 0 ensures that target_data_end == target_data_pos which causes the storage clear in clearStorageLoop() to be skipped.

[leonardoalt]

We should try this with the new code generator + optimizer

[Marenz]

We should try this with the new code generator + optimizer

I just tried it in remix and could not reproduce it anymore. (It uses the new backend as far as I can tell (because the debugger opened up a utilities.yul file))

Maybe we want to close this? Someone else could double check though, my remix skills are below-average at best.

[chriseth]

This might mean that this is part of the old abi decoder.

[cameel]

For the record: we're closing this because, as stated above, we did not manage to reproduce it with latest compiler so far. The issue pertains to a very old version of the compiler and is unlikely to be still relevant.