Skip to content

Commit ff0060c

Browse files
fanquakeD33r-Gee
fanquake
authored and
D33r-Gee
committed
Merge bitcoin/bitcoin#32520: Remove legacy Parse(U)Int*
faf55fc doc: Remove ParseInt mentions in documentation (MarcoFalke) 3333282 refactor: Remove unused Parse(U)Int* (MarcoFalke) fa84e6c bitcoin-tx: Reject + sign in MutateTxDel* (MarcoFalke) face251 bitcoin-tx: Reject + sign in vout parsing (MarcoFalke) fa8acaf bitcoin-tx: Reject + sign in replaceable parsing (MarcoFalke) faff25a bitcoin-tx: Reject + sign in locktime (MarcoFalke) dddd9e5 bitcoin-tx: Reject + sign in nversion parsing (MarcoFalke) fab06ac rest: Use SAFE_CHARS_URI in SanitizeString error msg (MarcoFalke) 8888bb4 rest: Reject + sign in /blockhashbyheight/ (MarcoFalke) fafd43c test: Reject + sign when parsing regtest deployment params (MarcoFalke) fa123af Reject + sign when checking -ipcfd (MarcoFalke) fa47985 Reject + sign in SplitHostPort (MarcoFalke) fab4c29 net: Reject + sign when parsing subnet mask (MarcoFalke) fa89652 init: Reject + sign in -*port parsing (MarcoFalke) fa9c455 cli: Reject + sign in -netinfo level parsing (MarcoFalke) fa98041 refactor: Use ToIntegral in CreateFromDump (MarcoFalke) fa23ed7 refactor: Use ToIntegral in ParseHDKeypath (MarcoFalke) Pull request description: The legacy int parsing is problematic, because it accepts the `+` sign for unsigned integers. In all cases this is either: * Useless, because the `+` sign was already rejected. * Erroneous and inconsistent, when third party parsers reject it. (C.f. bitcoin/bitcoin#32365) * Confusing, because the `+` sign is neither documented, nor can it be assumed to be present. Fix all issues by removing the legacy int parsing. ACKs for top commit: stickies-v: re-ACK faf55fc brunoerg: code review ACK faf55fc Tree-SHA512: a311ab6a58fe02a37741c1800feb3dcfad92377b4bfb61b433b2393f52ba89ef45d00940972b2767b213a3dd7b59e5e35d5b659c586eacdfe4e565a77b12b19f
2 parents 0f9baba + faf55fc commit ff0060c

20 files changed

+141
-395
lines changed

‎doc/developer-notes.md

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1009,10 +1009,6 @@ Strings and formatting
10091009
buffer overflows, and surprises with `\0` characters. Also, some C string manipulations
10101010
tend to act differently depending on platform, or even the user locale.
10111011
1012-
- Use `ToIntegral` from [`strencodings.h`](/src/util/strencodings.h) for number parsing. In legacy code you might also find `ParseInt*` family of functions, `ParseDouble` or `LocaleIndependentAtoi`.
1013-
1014-
- *Rationale*: These functions do overflow checking and avoid pesky locale issues.
1015-
10161012
- For `strprintf`, `LogInfo`, `LogDebug`, etc formatting characters don't need size specifiers.
10171013
10181014
- *Rationale*: Bitcoin Core uses tinyformat, which is type safe. Leave them out to avoid confusion.

‎src/bitcoin-cli.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
// Copyright (c) 2009-2010 Satoshi Nakamoto
2-
// Copyright (c) 2009-2022 The Bitcoin Core developers
2+
// Copyright (c) 2009-present The Bitcoin Core developers
33
// Distributed under the MIT software license, see the accompanying
44
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
55

@@ -472,7 +472,8 @@ class NetinfoRequestHandler : public BaseRequestHandler
472472
{
473473
if (!args.empty()) {
474474
uint8_t n{0};
475-
if (ParseUInt8(args.at(0), &n)) {
475+
if (const auto res{ToIntegral<uint8_t>(args.at(0))}) {
476+
n = *res;
476477
m_details_level = std::min(n, NETINFO_MAX_LEVEL);
477478
} else {
478479
throw std::runtime_error(strprintf("invalid -netinfo level argument: %s\nFor more information, run: bitcoin-cli -netinfo help", args.at(0)));

‎src/bitcoin-tx.cpp

Lines changed: 21 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -211,35 +211,33 @@ static CAmount ExtractAndValidateValue(const std::string& strValue)
211211

212212
static void MutateTxVersion(CMutableTransaction& tx, const std::string& cmdVal)
213213
{
214-
uint32_t newVersion;
215-
if (!ParseUInt32(cmdVal, &newVersion) || newVersion < 1 || newVersion > TX_MAX_STANDARD_VERSION) {
214+
const auto ver{ToIntegral<uint32_t>(cmdVal)};
215+
if (!ver || *ver < 1 || *ver > TX_MAX_STANDARD_VERSION) {
216216
throw std::runtime_error("Invalid TX version requested: '" + cmdVal + "'");
217217
}
218-
219-
tx.version = newVersion;
218+
tx.version = *ver;
220219
}
221220

222221
static void MutateTxLocktime(CMutableTransaction& tx, const std::string& cmdVal)
223222
{
224-
int64_t newLocktime;
225-
if (!ParseInt64(cmdVal, &newLocktime) || newLocktime < 0LL || newLocktime > 0xffffffffLL)
223+
const auto locktime{ToIntegral<uint32_t>(cmdVal)};
224+
if (!locktime) {
226225
throw std::runtime_error("Invalid TX locktime requested: '" + cmdVal + "'");
227-
228-
tx.nLockTime = (unsigned int) newLocktime;
226+
}
227+
tx.nLockTime = *locktime;
229228
}
230229

231230
static void MutateTxRBFOptIn(CMutableTransaction& tx, const std::string& strInIdx)
232231
{
233-
// parse requested index
234-
int64_t inIdx = -1;
235-
if (strInIdx != "" && (!ParseInt64(strInIdx, &inIdx) || inIdx < 0 || inIdx >= static_cast<int64_t>(tx.vin.size()))) {
232+
const auto idx{ToIntegral<uint32_t>(strInIdx)};
233+
if (strInIdx != "" && (!idx || *idx >= tx.vin.size())) {
236234
throw std::runtime_error("Invalid TX input index '" + strInIdx + "'");
237235
}
238236

239237
// set the nSequence to MAX_INT - 2 (= RBF opt in flag)
240-
int cnt = 0;
238+
uint32_t cnt{0};
241239
for (CTxIn& txin : tx.vin) {
242-
if (strInIdx == "" || cnt == inIdx) {
240+
if (strInIdx == "" || cnt == *idx) {
243241
if (txin.nSequence > MAX_BIP125_RBF_SEQUENCE) {
244242
txin.nSequence = MAX_BIP125_RBF_SEQUENCE;
245243
}
@@ -277,9 +275,10 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu
277275

278276
// extract and validate vout
279277
const std::string& strVout = vStrInputParts[1];
280-
int64_t vout;
281-
if (!ParseInt64(strVout, &vout) || vout < 0 || vout > static_cast<int64_t>(maxVout))
278+
const auto vout{ToIntegral<uint32_t>(strVout)};
279+
if (!vout || *vout > maxVout) {
282280
throw std::runtime_error("invalid TX input vout '" + strVout + "'");
281+
}
283282

284283
// extract the optional sequence number
285284
uint32_t nSequenceIn = CTxIn::SEQUENCE_FINAL;
@@ -288,7 +287,7 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu
288287
}
289288

290289
// append to transaction input list
291-
CTxIn txin(*txid, vout, CScript(), nSequenceIn);
290+
CTxIn txin{*txid, *vout, CScript{}, nSequenceIn};
292291
tx.vin.push_back(txin);
293292
}
294293

@@ -508,26 +507,20 @@ static void MutateTxAddOutScript(CMutableTransaction& tx, const std::string& str
508507

509508
static void MutateTxDelInput(CMutableTransaction& tx, const std::string& strInIdx)
510509
{
511-
// parse requested deletion index
512-
int64_t inIdx;
513-
if (!ParseInt64(strInIdx, &inIdx) || inIdx < 0 || inIdx >= static_cast<int64_t>(tx.vin.size())) {
510+
const auto idx{ToIntegral<uint32_t>(strInIdx)};
511+
if (!idx || idx >= tx.vin.size()) {
514512
throw std::runtime_error("Invalid TX input index '" + strInIdx + "'");
515513
}
516-
517-
// delete input from transaction
518-
tx.vin.erase(tx.vin.begin() + inIdx);
514+
tx.vin.erase(tx.vin.begin() + *idx);
519515
}
520516

521517
static void MutateTxDelOutput(CMutableTransaction& tx, const std::string& strOutIdx)
522518
{
523-
// parse requested deletion index
524-
int64_t outIdx;
525-
if (!ParseInt64(strOutIdx, &outIdx) || outIdx < 0 || outIdx >= static_cast<int64_t>(tx.vout.size())) {
519+
const auto idx{ToIntegral<uint32_t>(strOutIdx)};
520+
if (!idx || idx >= tx.vout.size()) {
526521
throw std::runtime_error("Invalid TX output index '" + strOutIdx + "'");
527522
}
528-
529-
// delete output from transaction
530-
tx.vout.erase(tx.vout.begin() + outIdx);
523+
tx.vout.erase(tx.vout.begin() + *idx);
531524
}
532525

533526
static const unsigned int N_SIGHASH_OPTS = 7;

‎src/chainparams.cpp

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -53,14 +53,14 @@ void ReadRegTestArgs(const ArgsManager& args, CChainParams::RegTestOptions& opti
5353
}
5454

5555
const auto value{arg.substr(found + 1)};
56-
int32_t height;
57-
if (!ParseInt32(value, &height) || height < 0 || height >= std::numeric_limits<int>::max()) {
56+
const auto height{ToIntegral<int32_t>(value)};
57+
if (!height || *height < 0 || *height >= std::numeric_limits<int>::max()) {
5858
throw std::runtime_error(strprintf("Invalid height value (%s) for -testactivationheight=name@height.", arg));
5959
}
6060

6161
const auto deployment_name{arg.substr(0, found)};
6262
if (const auto buried_deployment = GetBuriedDeployment(deployment_name)) {
63-
options.activation_heights[*buried_deployment] = height;
63+
options.activation_heights[*buried_deployment] = *height;
6464
} else {
6565
throw std::runtime_error(strprintf("Invalid name (%s) for -testactivationheight=name@height.", arg));
6666
}
@@ -72,16 +72,22 @@ void ReadRegTestArgs(const ArgsManager& args, CChainParams::RegTestOptions& opti
7272
throw std::runtime_error("Version bits parameters malformed, expecting deployment:start:end[:min_activation_height]");
7373
}
7474
CChainParams::VersionBitsParameters vbparams{};
75-
if (!ParseInt64(vDeploymentParams[1], &vbparams.start_time)) {
75+
const auto start_time{ToIntegral<int64_t>(vDeploymentParams[1])};
76+
if (!start_time) {
7677
throw std::runtime_error(strprintf("Invalid nStartTime (%s)", vDeploymentParams[1]));
7778
}
78-
if (!ParseInt64(vDeploymentParams[2], &vbparams.timeout)) {
79+
vbparams.start_time = *start_time;
80+
const auto timeout{ToIntegral<int64_t>(vDeploymentParams[2])};
81+
if (!timeout) {
7982
throw std::runtime_error(strprintf("Invalid nTimeout (%s)", vDeploymentParams[2]));
8083
}
84+
vbparams.timeout = *timeout;
8185
if (vDeploymentParams.size() >= 4) {
82-
if (!ParseInt32(vDeploymentParams[3], &vbparams.min_activation_height)) {
86+
const auto min_activation_height{ToIntegral<int64_t>(vDeploymentParams[3])};
87+
if (!min_activation_height) {
8388
throw std::runtime_error(strprintf("Invalid min_activation_height (%s)", vDeploymentParams[3]));
8489
}
90+
vbparams.min_activation_height = *min_activation_height;
8591
} else {
8692
vbparams.min_activation_height = 0;
8793
}

‎src/init.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1151,8 +1151,8 @@ bool CheckHostPortOptions(const ArgsManager& args) {
11511151
"-rpcport",
11521152
}) {
11531153
if (const auto port{args.GetArg(port_option)}) {
1154-
uint16_t n;
1155-
if (!ParseUInt16(*port, &n) || n == 0) {
1154+
const auto n{ToIntegral<uint16_t>(*port)};
1155+
if (!n || *n == 0) {
11561156
return InitError(InvalidPortErrMsg(port_option, *port));
11571157
}
11581158
}

‎src/interfaces/node.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include <logging.h>
1111
#include <net.h>
1212
#include <net_types.h>
13+
#include <node/utxo_snapshot.h>
1314
#include <netaddress.h>
1415
#include <netbase.h>
1516
#include <support/allocators/secure.h>
@@ -205,6 +206,9 @@ class Node
205206
//! List rpc commands.
206207
virtual std::vector<std::string> listRpcCommands() = 0;
207208

209+
//! Load and activate a snapshot file
210+
virtual bool loadSnapshot(AutoFile& coins_file, const node::SnapshotMetadata& metadata, bool in_memory) = 0;
211+
208212
//! Set RPC timer interface if unset.
209213
virtual void rpcSetTimerInterfaceIfUnset(RPCTimerInterface* iface) = 0;
210214

‎src/ipc/process.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,11 @@ class ProcessImpl : public Process
5555
// in combination with other arguments because the parent process
5656
// should be able to control the child process through the IPC protocol
5757
// without passing information out of band.
58-
if (!ParseInt32(argv[2], &fd)) {
58+
const auto maybe_fd{ToIntegral<int32_t>(argv[2])};
59+
if (!maybe_fd) {
5960
throw std::runtime_error(strprintf("Invalid -ipcfd number '%s'", argv[2]));
6061
}
62+
fd = *maybe_fd;
6163
return true;
6264
}
6365
int connect(const fs::path& data_dir,

‎src/netbase.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -829,10 +829,9 @@ CSubNet LookupSubNet(const std::string& subnet_str)
829829
addr = static_cast<CNetAddr>(MaybeFlipIPv6toCJDNS(CService{addr.value(), /*port=*/0}));
830830
if (slash_pos != subnet_str.npos) {
831831
const std::string netmask_str{subnet_str.substr(slash_pos + 1)};
832-
uint8_t netmask;
833-
if (ParseUInt8(netmask_str, &netmask)) {
832+
if (const auto netmask{ToIntegral<uint8_t>(netmask_str)}) {
834833
// Valid number; assume CIDR variable-length subnet masking.
835-
subnet = CSubNet{addr.value(), netmask};
834+
subnet = CSubNet{addr.value(), *netmask};
836835
} else {
837836
// Invalid number; try full netmask syntax. Never allow lookup for netmask.
838837
const std::optional<CNetAddr> full_netmask{LookupHost(netmask_str, /*fAllowLookup=*/false)};

‎src/node/interfaces.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,11 @@ class NodeImpl : public Node
355355
return ::tableRPC.execute(req);
356356
}
357357
std::vector<std::string> listRpcCommands() override { return ::tableRPC.listCommands(); }
358+
bool loadSnapshot(AutoFile& coins_file, const SnapshotMetadata& metadata, bool in_memory) override
359+
{
360+
auto activation_result{chainman().ActivateSnapshot(coins_file, metadata, in_memory)};
361+
return activation_result.has_value();
362+
}
358363
void rpcSetTimerInterfaceIfUnset(RPCTimerInterface* iface) override { RPCSetTimerInterfaceIfUnset(iface); }
359364
void rpcUnsetTimerInterface(RPCTimerInterface* iface) override { RPCUnsetTimerInterface(iface); }
360365
std::optional<Coin> getUnspentOutput(const COutPoint& output) override

‎src/rest.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -962,9 +962,9 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,
962962
std::string height_str;
963963
const RESTResponseFormat rf = ParseDataFormat(height_str, str_uri_part);
964964

965-
int32_t blockheight = -1; // Initialization done only to prevent valgrind false positive, see https://github.com/bitcoin/bitcoin/pull/18785
966-
if (!ParseInt32(height_str, &blockheight) || blockheight < 0) {
967-
return RESTERR(req, HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str));
965+
const auto blockheight{ToIntegral<int32_t>(height_str)};
966+
if (!blockheight || *blockheight < 0) {
967+
return RESTERR(req, HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str, SAFE_CHARS_URI));
968968
}
969969

970970
CBlockIndex* pblockindex = nullptr;
@@ -974,10 +974,10 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,
974974
ChainstateManager& chainman = *maybe_chainman;
975975
LOCK(cs_main);
976976
const CChain& active_chain = chainman.ActiveChain();
977-
if (blockheight > active_chain.Height()) {
977+
if (*blockheight > active_chain.Height()) {
978978
return RESTERR(req, HTTP_NOT_FOUND, "Block height out of range");
979979
}
980-
pblockindex = active_chain[blockheight];
980+
pblockindex = active_chain[*blockheight];
981981
}
982982
switch (rf) {
983983
case RESTResponseFormat::BINARY: {

0 commit comments

Comments
 (0)