Changeset View
Changeset View
Standalone View
Standalone View
src/script/interpreter.cpp
| Show First 20 Lines • Show All 955 Lines • ▼ Show 20 Lines | try { | ||||
| return set_error(serror, | return set_error(serror, | ||||
| SCRIPT_ERR_CHECKDATASIGVERIFY); | SCRIPT_ERR_CHECKDATASIGVERIFY); | ||||
| } | } | ||||
| } | } | ||||
| } break; | } break; | ||||
| case OP_CHECKMULTISIG: | case OP_CHECKMULTISIG: | ||||
| case OP_CHECKMULTISIGVERIFY: { | case OP_CHECKMULTISIGVERIFY: { | ||||
| // ([sig ...] num_of_signatures [pubkey ...] | // ([dummy] [sig ...] num_of_signatures [pubkey ...] | ||||
| // num_of_pubkeys -- bool) | // num_of_pubkeys -- bool) | ||||
| if (stack.size() < 1) { | |||||
| int i = 1; | |||||
| if ((int)stack.size() < i) { | |||||
| return set_error( | return set_error( | ||||
| serror, SCRIPT_ERR_INVALID_STACK_OPERATION); | serror, SCRIPT_ERR_INVALID_STACK_OPERATION); | ||||
| } | } | ||||
| int nKeysCount = | const int nKeysCount = | ||||
| CScriptNum(stacktop(-i), fRequireMinimal).getint(); | CScriptNum(stacktop(-1), fRequireMinimal).getint(); | ||||
deadalnix: Then 1 is the index of nKeysCount and the stack size check ensure that this index is within… | |||||
| if (nKeysCount < 0 || | if (nKeysCount < 0 || | ||||
| nKeysCount > MAX_PUBKEYS_PER_MULTISIG) { | nKeysCount > MAX_PUBKEYS_PER_MULTISIG) { | ||||
| return set_error(serror, SCRIPT_ERR_PUBKEY_COUNT); | return set_error(serror, SCRIPT_ERR_PUBKEY_COUNT); | ||||
| } | } | ||||
| nOpCount += nKeysCount; | nOpCount += nKeysCount; | ||||
| if (nOpCount > MAX_OPS_PER_SCRIPT) { | if (nOpCount > MAX_OPS_PER_SCRIPT) { | ||||
| return set_error(serror, SCRIPT_ERR_OP_COUNT); | return set_error(serror, SCRIPT_ERR_OP_COUNT); | ||||
| } | } | ||||
| int ikey = ++i; | |||||
| // ikey2 is the position of last non-signature item in | // position of the top pubkey on stack | ||||
| // the stack. Top stack item = 1. With | const int idxTopKey = 2; | ||||
deadalnixUnsubmitted Presumably, that would be the element immediately after the index of nKeysCount. deadalnix: Presumably, that would be the element immediately after the index of nKeysCount. | |||||
| // SCRIPT_VERIFY_NULLFAIL, this is used for cleanup if | |||||
| // operation fails. | // idxSigCount the position of nSigsCount, the stack | ||||
| int ikey2 = nKeysCount + 2; | // element that sits between the signatures list and | ||||
| i += nKeysCount; | // pubkeys list. | ||||
| if ((int)stack.size() < i) { | const int idxSigCount = idxTopKey + nKeysCount; | ||||
| if (stack.size() < size_t(idxSigCount)) { | |||||
deadalnixUnsubmitted Making the index size_t as they should be avoid the need for this. deadalnix: Making the index size_t as they should be avoid the need for this. | |||||
markblundebergAuthorUnsubmitted Some of these indices get used in arithmetic with ints, for example -idxTopSig - k below ... it's not immediately apparent to me that it maintains the same behaviour if the indices are now size_t. Again, I'd like to defer the size_t thing to a later diff. markblundeberg: Some of these indices get used in arithmetic with ints, for example `-idxTopSig - k` below ... | |||||
deadalnixUnsubmitted You are introducing new variables, make them the right type. This does the same thing. There is no such thing as a signed or an unsigned add for the CPU: https://c9x.me/x86/html/file_module_x86_id_5.html . size_t is the correct type here. Get familiar with 2 complement math if you are not convinced, but I can assure you this is the case. CPU do subtraction by doing via A - B = A + ~B + 1, so it's all additions. The only thing you'll get using an int is to risk the compiler to insert masks in various places to ensure things fit on 32 bits, and the optimizer will assuming that arithmetic operation do not overflow, which you usualy don't want. deadalnix: You are introducing new variables, make them the right type.
This does the same thing. There… | |||||
markblundebergAuthorUnsubmitted I know how two's complement arithmetic works, I'm just concerned about type casts since from what I read, an int can in principle be smaller, equal, or larger in size than a size_t in different architectures. I'm not convinced that an int is wrong here, or that a size_t is right. markblundeberg: I know how two's complement arithmetic works, I'm just concerned about type casts since from… | |||||
deadalnixUnsubmitted Every single use is either used to compare with the size of a vector or index in a vector. This is 100% a size_t . Doing the wrong thing in the consensus code to match deadline is definitively the wrong tradeof The only way to not get impacted by delay is to start earlier. deadalnix: Every single use is either used to compare with the size of a vector or index in a vector. This… | |||||
| return set_error( | return set_error( | ||||
| serror, SCRIPT_ERR_INVALID_STACK_OPERATION); | serror, SCRIPT_ERR_INVALID_STACK_OPERATION); | ||||
| } | } | ||||
| const int nSigsCount = | |||||
| int nSigsCount = | CScriptNum(stacktop(-idxSigCount), fRequireMinimal) | ||||
| CScriptNum(stacktop(-i), fRequireMinimal).getint(); | .getint(); | ||||
| if (nSigsCount < 0 || nSigsCount > nKeysCount) { | if (nSigsCount < 0 || nSigsCount > nKeysCount) { | ||||
| return set_error(serror, SCRIPT_ERR_SIG_COUNT); | return set_error(serror, SCRIPT_ERR_SIG_COUNT); | ||||
| } | } | ||||
| int isig = ++i; | |||||
| i += nSigsCount; | // position of the top signature on stack | ||||
| if ((int)stack.size() < i) { | const int idxTopSig = idxSigCount + 1; | ||||
| // position of the dummy element on stack | |||||
| const int idxDummy = idxTopSig + nSigsCount; | |||||
| if (stack.size() < size_t(idxDummy)) { | |||||
deadalnixUnsubmitted This changes the semantic. Please do it in another diff. deadalnix: This changes the semantic. Please do it in another diff. | |||||
markblundebergAuthorUnsubmitted Can you elaborate on what has changed? Do you mean the change from i to idxDummy? At this point, i points to the dummy element, so nothing has changed. markblundeberg: Can you elaborate on what has changed?
Do you mean the change from `i` to `idxDummy`? At this… | |||||
deadalnixUnsubmitted ok deadalnix: ok | |||||
| return set_error( | return set_error( | ||||
| serror, SCRIPT_ERR_INVALID_STACK_OPERATION); | serror, SCRIPT_ERR_INVALID_STACK_OPERATION); | ||||
| } | } | ||||
| // Subset of script starting at the most recent | // Subset of script starting at the most recent | ||||
| // codeseparator | // codeseparator | ||||
| CScript scriptCode(pbegincodehash, pend); | CScript scriptCode(pbegincodehash, pend); | ||||
| // Remove signature for pre-fork scripts | // Remove signature for pre-fork scripts | ||||
| for (int k = 0; k < nSigsCount; k++) { | for (int k = 0; k < nSigsCount; k++) { | ||||
| valtype &vchSig = stacktop(-isig - k); | valtype &vchSig = stacktop(-idxTopSig - k); | ||||
| CleanupScriptCode(scriptCode, vchSig, flags); | CleanupScriptCode(scriptCode, vchSig, flags); | ||||
| } | } | ||||
| bool fSuccess = true; | bool fSuccess = true; | ||||
| while (fSuccess && nSigsCount > 0) { | int ikey = idxTopKey; | ||||
| int isig = idxTopSig; | |||||
| int nSigsRemaining = nSigsCount; | |||||
| int nKeysRemaining = nKeysCount; | |||||
| while (fSuccess && nSigsRemaining > 0) { | |||||
| valtype &vchSig = stacktop(-isig); | valtype &vchSig = stacktop(-isig); | ||||
| valtype &vchPubKey = stacktop(-ikey); | valtype &vchPubKey = stacktop(-ikey); | ||||
| // Note how this makes the exact order of | // Note how this makes the exact order of | ||||
| // pubkey/signature evaluation distinguishable by | // pubkey/signature evaluation distinguishable by | ||||
| // CHECKMULTISIG NOT if the STRICTENC flag is set. | // CHECKMULTISIG NOT if the STRICTENC flag is set. | ||||
| // See the script_(in)valid tests for details. | // See the script_(in)valid tests for details. | ||||
| if (!CheckTransactionECDSASignatureEncoding( | if (!CheckTransactionECDSASignatureEncoding( | ||||
| vchSig, flags, serror) || | vchSig, flags, serror) || | ||||
| !CheckPubKeyEncoding(vchPubKey, flags, | !CheckPubKeyEncoding(vchPubKey, flags, | ||||
| serror)) { | serror)) { | ||||
| // serror is set | // serror is set | ||||
| return false; | return false; | ||||
| } | } | ||||
| // Check signature | // Check signature | ||||
| bool fOk = checker.CheckSig(vchSig, vchPubKey, | bool fOk = checker.CheckSig(vchSig, vchPubKey, | ||||
| scriptCode, flags); | scriptCode, flags); | ||||
| if (fOk) { | if (fOk) { | ||||
| isig++; | isig++; | ||||
| nSigsCount--; | nSigsRemaining--; | ||||
| } | } | ||||
| ikey++; | ikey++; | ||||
| nKeysCount--; | nKeysRemaining--; | ||||
| // If there are more signatures left than keys left, | // If there are more signatures left than keys left, | ||||
| // then too many signatures have failed. Exit early, | // then too many signatures have failed. Exit early, | ||||
| // without checking any further signatures. | // without checking any further signatures. | ||||
| if (nSigsCount > nKeysCount) { | if (nSigsRemaining > nKeysRemaining) { | ||||
| fSuccess = false; | fSuccess = false; | ||||
| } | } | ||||
| } | } | ||||
deadalnixUnsubmitted That loop is still horribly screwed up, but at least the screwing up is now limited to the loop.
deadalnix: That loop is still horribly screwed up, but at least the screwing up is now limited to the loop. | |||||
| // Clean up stack of actual arguments | // Clean up stack of actual arguments | ||||
| int i = idxDummy; | |||||
| int ikey2 = idxSigCount; | |||||
| while (i-- > 1) { | while (i-- > 1) { | ||||
| // If the operation failed, we require that all | // If the operation failed, we require that all | ||||
| // signatures must be empty vector | // signatures must be empty vector | ||||
| if (!fSuccess && (flags & SCRIPT_VERIFY_NULLFAIL) && | if (!fSuccess && (flags & SCRIPT_VERIFY_NULLFAIL) && | ||||
| !ikey2 && stacktop(-1).size()) { | !ikey2 && stacktop(-1).size()) { | ||||
| return set_error(serror, | return set_error(serror, | ||||
| SCRIPT_ERR_SIG_NULLFAIL); | SCRIPT_ERR_SIG_NULLFAIL); | ||||
| } | } | ||||
| if (ikey2 > 0) { | if (ikey2 > 0) { | ||||
| ikey2--; | ikey2--; | ||||
| } | } | ||||
| popstack(stack); | popstack(stack); | ||||
| } | } | ||||
| // A bug causes CHECKMULTISIG to consume one extra | // A bug causes CHECKMULTISIG to consume one extra | ||||
| // argument whose contents were not checked in any way. | // argument whose contents were not checked in any way. | ||||
| // | // | ||||
| // Unfortunately this is a potential source of | // Unfortunately this is a potential source of | ||||
| // mutability, so optionally verify it is exactly equal | // mutability, so optionally verify it is exactly equal | ||||
| // to zero prior to removing it from the stack. | // to zero prior to removing it from the stack. | ||||
| if (stack.size() < 1) { | |||||
| return set_error( | |||||
| serror, SCRIPT_ERR_INVALID_STACK_OPERATION); | |||||
| } | |||||
deadalnixUnsubmitted Extract this in another diff. deadalnix: Extract this in another diff. | |||||
| if ((flags & SCRIPT_VERIFY_NULLDUMMY) && | if ((flags & SCRIPT_VERIFY_NULLDUMMY) && | ||||
| stacktop(-1).size()) { | stacktop(-1).size()) { | ||||
| return set_error(serror, SCRIPT_ERR_SIG_NULLDUMMY); | return set_error(serror, SCRIPT_ERR_SIG_NULLDUMMY); | ||||
| } | } | ||||
| popstack(stack); | popstack(stack); | ||||
| stack.push_back(fSuccess ? vchTrue : vchFalse); | stack.push_back(fSuccess ? vchTrue : vchFalse); | ||||
| if (opcode == OP_CHECKMULTISIGVERIFY) { | if (opcode == OP_CHECKMULTISIGVERIFY) { | ||||
| if (fSuccess) { | if (fSuccess) { | ||||
| popstack(stack); | popstack(stack); | ||||
| } else { | } else { | ||||
| return set_error( | return set_error( | ||||
| serror, SCRIPT_ERR_CHECKMULTISIGVERIFY); | serror, SCRIPT_ERR_CHECKMULTISIGVERIFY); | ||||
| } | } | ||||
| } | } | ||||
| ▲ Show 20 Lines • Show All 578 Lines • Show Last 20 Lines | |||||
Then 1 is the index of nKeysCount and the stack size check ensure that this index is within bounds, so it should use it as well.