CScript: remove redundant bounds check #9451

pull robmcl4 wants to merge 1 commits into bitcoin:master from robmcl4:remove_extra_bounds_check changing 1 files +1 −2

robmcl4 commented at 6:09 PM on December 31, 2016: contributor

CScript::GetOp2(..) performed two equivalent bounds checks, one was removed as redundant.

remove redundant bounds check

CScript::GetOp2(..) performed two equivalent
bounds checks, one was removed as redundant

17d837b6ba

fanquake added the label Validation on Jan 1, 2017
cdecker commented at 10:48 AM on January 2, 2017: contributor

ACK 17d837b6bacf58462e4a6351731a87d8362fef6b
in src/script/script.h:None in 17d837b6ba
```
 507 | @@ -508,8 +508,7 @@ class CScript : public CScriptBase
 508 |              return false;
 509 |  
 510 |          // Read instruction
 511 | -        if (end() - pc < 1)
 512 | -            return false;
```
dcousens commented at 12:08 AM on January 3, 2017:

Provided pc is a valid iterator, of this, then yes, this is redundant. I'm not 100% across this code to know if that assumption is true.

sipa commented at 10:23 PM on January 3, 2017:

If it's not a valid iterator, just subtracting it from end() is probably undefined behaviour.
dcousens approved
laanwj commented at 10:28 AM on January 5, 2017: member

Is there any (quantifiable) performance reason to remove this?

If not, I wouldn't personally mind keeping the extraneous check as belt-and-suspender, and not change the consensus code unnecessarily.
cdecker commented at 1:00 PM on January 5, 2017: contributor

Is there an argument to be made for merging the two checks into a single if? That'd at least make it somewhat obvious that it's checking the same thing.
pstratem commented at 1:13 PM on January 5, 2017: contributor

@laanwj i believe this actually did something in the very distant past when multi byte opcodes were supported (like 2009 distant past)

fanquake commented at 1:54 PM on January 5, 2017: member

This is as far back as I'm looking https://github.com/bitcoin/bitcoin/commit/e071a3f6c06f41068ad17134189a4ac3073ef76b:

    bool GetOp(const_iterator& pc, opcodetype& opcodeRet, vector<unsigned char>& vchRet) const
    {
        opcodeRet = OP_INVALIDOPCODE;
        vchRet.clear();
        if (pc >= end())
            return false;

        // Read instruction
        unsigned int opcode = *pc++;
        if (opcode >= OP_SINGLEBYTE_END)
        {
            if (pc + 1 > end())
                return false;
            opcode <<= 8;
            opcode |= *pc++;
        }

        // Immediate operand
        if (opcode <= OP_PUSHDATA4)
        {
            unsigned int nSize = opcode;
            if (opcode == OP_PUSHDATA1)
            {
                if (pc + 1 > end())
                    return false;
                nSize = *pc++;
            }
            else if (opcode == OP_PUSHDATA2)
            {
                if (pc + 2 > end())
                    return false;
                nSize = 0;
                memcpy(&nSize, &pc[0], 2);
                pc += 2;
            }
            else if (opcode == OP_PUSHDATA4)
            {
                if (pc + 4 > end())
                    return false;
                memcpy(&nSize, &pc[0], 4);
                pc += 4;
            }
            if (pc + nSize > end())
                return false;
            vchRet.assign(pc, pc + nSize);
            pc += nSize;
        }

        opcodeRet = (opcodetype)opcode;
        return true;
    }

laanwj commented at 2:10 PM on January 5, 2017: member

Is there an argument to be made for merging the two checks into a single if? That'd at least make it somewhat obvious that it's checking the same thing.

Sure - though the same could be achieved with a comment, documenting that there is a duplicate bounds check here. This could then be removed later, or not, but doesn't require a consensus code change now. @pstratem @fanquake seems indeed it's been there for really long
fanquake commented at 12:27 AM on January 21, 2017: member

Closing. Feel free to re-open with a change documenting the duplicate, but I don't think a consensus code change will be merged right now.
fanquake closed this on Jan 21, 2017
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels