Age | Commit message (Collapse) | Author |
|
Add more fs::path operator/ and operator+ overloads to prevent unsafe
string->path conversions on Windows that would cause strings to be
decoded according to the current Windows locale & code page instead of
the correct string encoding.
Update application code to deal with loss of implicit string->path
conversions by calling fs::u8path or fs::PathFromString explicitly, or
by just changing variable types from std::string to fs::path to avoid
conversions altoghther, or make them happen earlier.
In all cases, there's no change in behavior either (1) because strings
only contained ASCII characters and would be decoded the same regardless
of what encoding was used, or (2) because of the 1:1 mapping between
paths and strings using the PathToString and PathFromString functions.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
|
|
3ae7791bcaa88f5c68592673b8926ee807242ce7 refactor: use Span in random.* (pasta)
Pull request description:
~This PR does two things~
1. use a Span<unsigned char> for GetRandBytes and GetStrongRandBytes
~2. make GetRand a template for which any integral type can be used, where the default behavior is to return a random integral up to the max of the integral unless a max is provided.
This simplifies a lot of code from `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()`~
MarcoFalke this was inspired by your comment here: https://github.com/bitcoin/bitcoin/pull/24185#issuecomment-1025514263 about using Span, so hopefully I'll be able to get this PR done and merged 😂
~Also, if requested I could revert the `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()` related changes if it ends up causing too many conflicts~
ACKs for top commit:
laanwj:
Thank you! Code review re-ACK 3ae7791bcaa88f5c68592673b8926ee807242ce7
Tree-SHA512: 12375a83b68b288916ba0de81cfcab4aac14389a66a36811ae850427435eb67dd55e47df9ac3ec47db4e214f4330139e548bec815fff8a3f571484ea558dca79
|
|
Removes unhelpful noise/verbosity.
See: https://github.com/bitcoin/bitcoin/pull/24306#discussion_r809363741
|
|
Also "includeconf" values been normalized.
|
|
SetSocketNoDelay() mockable/testable
a2c4a7acd1dfb2fb7e3c9dac6b3d8c9354b2e0a6 net: use Sock::SetSockOpt() instead of standalone SetSocketNoDelay() (Vasil Dimov)
d65b6c3fb9cdd41fa53bc76a7b8f49aaa089b0bc net: use Sock::SetSockOpt() instead of setsockopt() (Vasil Dimov)
184e56d6683d05fc84f5153cfff83a2e32883556 net: add new method Sock::SetSockOpt() that wraps setsockopt() (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Add a `virtual` (thus mockable) method `Sock::SetSockOpt()` that wraps the system `setsockopt()`.
Convert the standalone `SetSocketNoDelay()` function to a `virtual` (thus mockable) method `Sock::SetNoDelay()`.
This will help avoid syscalls during testing and to mock them to return whatever is suitable for the tests.
ACKs for top commit:
laanwj:
Code review ACK a2c4a7acd1dfb2fb7e3c9dac6b3d8c9354b2e0a6
jonatack:
ACK a2c4a7acd1dfb2fb7e3c9dac6b3d8c9354b2e0a6 change since last review is folding `Sock::SetNoDelay()` into the callers
Tree-SHA512: 3e2b016c1e4128317a28c17dc9b30472949e1ac3b071b2697c6d30cbcc830df1ee4392a4e23b2ea1ab4e3fb0f59ef450e2a4f3c1df3d8c803dd081652b6c7387
|
|
In my benchmark, this rewrite improves runtime 27% (g++) to 46% (clang++) for the benchmark `HexStrBench`:
g++ 11.2.0
| ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 0.94 | 1,061,381,310.36 | 0.7% | 12.00 | 3.01 | 3.990 | 1.00 | 0.0% | 0.01 | `HexStrBench` master
| 0.68 | 1,465,366,544.25 | 1.7% | 6.00 | 2.16 | 2.778 | 1.00 | 0.0% | 0.01 | `HexStrBench` branch
clang++ 13.0.1
| ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 0.80 | 1,244,713,415.92 | 0.9% | 10.00 | 2.56 | 3.913 | 0.50 | 0.0% | 0.01 | `HexStrBench` master
| 0.43 | 2,324,188,940.72 | 0.2% | 4.00 | 1.37 | 2.914 | 0.25 | 0.0% | 0.01 | `HexStrBench` branch
Note that the idea for this change comes from denis2342 in PR 23364. This is a rewrite so no unaligned accesses occur.
Also, the lookup table is now calculated at compile time, which hopefully makes the code a bit easier to review.
|
|
UNREACHABLE macros
|
|
|
|
This will help to increase `Sock` usage and make more code mockable.
|
|
This helper uses spanparsing::Split internally and enables to replace
all calls to boost::split where only a single separator is passed.
Co-authored-by: Martin Ankerl <Martin.Ankerl@gmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
|
|
* Revert "util: Add inotify_rm_watch to syscall sandbox (AllowFileSystem)"
This reverts commit f05a4cdf5a0363e1c12f00c034afb60e7ea0c775.
* Revert "util: add linkat to syscall sandbox (AllowFileSystem)"
This reverts commit 9809db3577f0fa618bea42635b1581e628a30395.
|
|
|
|
1633f5ec8846408182cceb60dc88f022635f4002 util, refactor: Add UNIQUE_NAME helper macro (Hennadii Stepanov)
Pull request description:
This PR replaces repetitive code with a helper macro.
ACKs for top commit:
laanwj:
Tested ACK 1633f5ec8846408182cceb60dc88f022635f4002
Tree-SHA512: 5f04e472c5f3184c0a9df75395377c6744bfb2cd8f95f8427c1c5e20daa7d6a9b29e45424b88391fc6326d365907a750ab50fda534b49d1df80dccf0e18467a4
|
|
0add4dbadbc972933b0c99813a155a4ed4852975 util/check: avoid unused parameter warnings (Anthony Towns)
Pull request description:
Add `[[maybe_unused]]` annotations to avoid warnings from gcc 9.4 and earlier which don't analyse `if constexpr` properly.
ACKs for top commit:
MarcoFalke:
review ACK 0add4dbadbc972933b0c99813a155a4ed4852975
jonatack:
ACK 0add4dbadbc972933b0c99813a155a4ed4852975 review and debug build on clang 15
shaavan:
ACK 0add4dbadbc972933b0c99813a155a4ed4852975
Tree-SHA512: 3ba490d74d91692c1d22b927da43a130c92cd6a20ed168573e4fbe1f4675fef7e05ebf0b11f2bbd15da3c606fea1f8e6403cfca347009b8b6acc1e77bbee9963
|
|
|
|
By removing the whole comment. These #include // For comments are near impossible
to maintain, pollute diffs, and generally don't add a lot of value.
|
|
|
|
2ef47ba6c57a12840499a13908ab61aefca6cb55 util/check: stop using lambda for Assert/Assume (Anthony Towns)
7c9fe25c16d48b53a61fa2f6ff77eaf8820cb1f6 wallet: move Assert() check into constructor (Anthony Towns)
Pull request description:
Using a lambda creates a couple of odd namespacing issues, in particular making clang's thread safety analysis less helpful, and confusing gcc when calling member functions. Fix this by not using a lambda.
Fixes #21596
Fixes #24654
ACKs for top commit:
MarcoFalke:
ACK 2ef47ba6c57a12840499a13908ab61aefca6cb55 🚢
jonatack:
Tested re-ACK 2ef47ba6c57a12840499a13908ab61aefca6cb55
Tree-SHA512: 4bdbf3215f3d14472df0552362c5eebe8b7eea2d0928a8a41109edd4e0c5f95de6f8220eb2fee8506874e352c003907faf5ef344174795939306a618157b1bae
|
|
|
|
(AllowFileSystem)
f05a4cdf5a0363e1c12f00c034afb60e7ea0c775 util: Add inotify_rm_watch to syscall sandbox (AllowFileSystem) (Hennadii Stepanov)
Pull request description:
This PR fixes the current master (3297f5c11c72dd83479ff8335e047555e3f8cb3b) when running `bitcoin-qt` on Ubuntu 22.04 and quitting:
```
$ ./src/qt/bitcoin-qt -signet -sandbox=log-and-abort
Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway.
ERROR: The syscall "inotify_rm_watch" (syscall number 255) is not allowed by the syscall sandbox in thread "main". Please report.
terminate called without an active exception
Aborted (core dumped)
```
Also see https://github.com/bitcoin/bitcoin/pull/24659#discussion_r835747166
ACKs for top commit:
fanquake:
ACK f05a4cdf5a0363e1c12f00c034afb60e7ea0c775 - checked that qt is using this in it's filesystem watcher code.
Tree-SHA512: 9c7920a25422cd3a040bc1cbc487c12c3dc2b91358c3757f1030d6a1ff12c18c688a8e5b7466f683da88a5e4f5f15d442975660022d706e47021253c24c58f4a
|
|
532c64a7264dd3c7329e8839547837c57da7dbe8 build: Fix Boost.Process test for Boost 1.78 (Hennadii Stepanov)
Pull request description:
Rebased #24415 with Luke's suggestion.
Fixes #24413.
ACKs for top commit:
hebasto:
ACK 532c64a7264dd3c7329e8839547837c57da7dbe8, tested on Mac mini (M1, 2020) + macOS Monterey 12.3 (21E230).
Tree-SHA512: 74f779695f6bbc45a2b7341a1402f747cc0d433d74825c7196cb9f156db0c0299895365f01665bd0bff12a8ebb5ea33a29b9a52f5eac0007ec35d1dca6544705
|
|
|
|
|
|
|
|
|
|
a84650ebd5ac2cbb49f14eb7c98736a3f8215bf1 util: Fix ReadBinaryFile reading beyond maxsize (klementtan)
Pull request description:
Currently `ReadBinaryFile` will read beyond `maxsize` if `maxsize` is not a multiple of `128` (size of buffer)
This is due to `fread` being called with `count = 128` instead of `count = min(128, maxsize - retval.size()` at every iteration
The following unit test will fail:
```cpp
BOOST_AUTO_TEST_CASE(util_ReadWriteFile)
{
fs::path tmpfolder = m_args.GetDataDirBase();
fs::path tmpfile = tmpfolder / "read_binary.dat";
std::string expected_text(300,'c');
{
std::ofstream file{tmpfile};
file << expected_text;
}
{
// read half the contents in file
auto [valid, text] = ReadBinaryFile(tmpfile, expected_text.size() / 2);
BOOST_CHECK_EQUAL(text.size(), 150);
}
}
```
Error:
```
test/util_tests.cpp:2593: error: in "util_tests/util_ReadWriteFile": check text.size() == 150 has failed [256 != 150]
```
ACKs for top commit:
laanwj:
Code review ACK a84650ebd5ac2cbb49f14eb7c98736a3f8215bf1
theStack:
Code-review ACK a84650ebd5ac2cbb49f14eb7c98736a3f8215bf1
Tree-SHA512: 752eebe58bc2102dec199b6775f8c3304d899f0ce36d6a022a58e27b076ba945ccd572858b19137b769effd8c6de73a9277f641be24dfb17657fb7173ea0eda0
|
|
Fix GUI startup crash reported by Rspigler in
https://github.com/bitcoin/bitcoin/issues/24457 that happens if
settings.json contains an integer value for any of the configuration
options which GUI settings can currently clash with (-dbcache, -par,
-spendzeroconfchange, -signer, -upnp, -natpmp, -listen, -server, -proxy,
-proxy, -onion, -onion, -lang, and -prune).
Fix is a one-line change in ArgsManager::GetArg.
|
|
Take advantage of GetPathArg to simplify code slightly.
|
|
Let GetPathArg method be used more places for path arguments that have
default values, like "-settings" and BITCOIN_SETTINGS_FILENAME in the
next commit.
Also:
- Fix negated argument handling. Return path{} not path{"0"} when path
argument is negated.
- Add new tests for default and negated cases
- Move GetPathArg() method declaration next to GetArg() declarations.
The two methods are close substitutes for each other, so this should
help keep them consistent and make them more discoverable.
|
|
|
|
|
|
overflows
fa1b89a6bdbab50bdb0504782afd4bb3375d1b57 scripted-diff: Rename nReadPos to m_read_pos in streams.h (MarcoFalke)
fa56c79df91e5d87533af38b64f4f4148a48a276 Make CDataStream work properly on 64-bit systems (MarcoFalke)
fab02f799194c75af7def3a2ab45c443b75de230 streams: Fix read-past-the-end and integer overflows (MarcoFalke)
Pull request description:
This is a follow-up to commit e26b62093ae21e89ed7d36a24a6b863f38ec631d with the following fixes:
* Fix unsigned integer overflow in `ignore()`, when `nReadPos` wraps.
* Fix unsigned integer overflow in `read()`, when `nReadPos` wraps.
* Fix read-past-the-end in `read()`, when `nReadPos` wraps.
This shouldn't be remote-exploitable, because it requires a stream of more than 1GB of size. However, it might be exploitable if the attacker controls the datadir (I haven't checked).
A unit test for the overflow in `ignore()` looks like following. It is left as an excercise to the reader to replace `foo.ignore(7)` with the appropriate call to `read()` to reproduce the overflow and read-error in `read()`.
```diff
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index 922fd8e513..ec6ea93919 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -534,6 +534,20 @@ BOOST_AUTO_TEST_CASE(ccoins_serialization)
} catch (const std::ios_base::failure&) {
}
+ CDataStream foo{0, 0};
+ auto size{std::numeric_limits<uint32_t>::max()};
+ foo.resize(size);
+ BOOST_CHECK_EQUAL(foo.size(), size);
+ foo.ignore(std::numeric_limits<int32_t>::max());
+ size -= std::numeric_limits<int32_t>::max();
+ BOOST_CHECK_EQUAL(foo.size(), size);
+ foo.ignore(std::numeric_limits<int32_t>::max());
+ size -= std::numeric_limits<int32_t>::max();
+ BOOST_CHECK_EQUAL(foo.size(), size);
+ BOOST_CHECK_EQUAL(foo.size(), 1);
+ foo.ignore(7); // Should overflow, as the size is only 1
+ BOOST_CHECK_EQUAL(foo.size(), uint32_t(1 - 7));
+
// Very large scriptPubKey (3*10^9 bytes) past the end of the stream
CDataStream tmp(SER_DISK, CLIENT_VERSION);
uint64_t x = 3000000000ULL;
```
ACKs for top commit:
klementtan:
Code Review ACK fa1b89a6bdbab50bdb0504782afd4bb3375d1b57:
Tree-SHA512: 67f0a1baafe88eaf1dc844ac55b638d5cf168a18c945e3bf7a2cb03c9a5976674a8e3af2487d8a2c3eae21e5c0e7a519c8b16ee7f104934442e2769d100660e9
|
|
|
|
Fixes #24368.
|
|
dc01cbc538765f64326bca30952c83e3862d0d54 test: Add fs_tests/rename unit test (Hennadii Stepanov)
d4999d40b9bd04dc20111aaaa6ed2d3db1a5caf9 util: Revert back MoveFileExW call for MinGW-w64 (Hennadii Stepanov)
Pull request description:
Unfortunately, bitcoin/bitcoin#24308 introduced a [regression](https://github.com/bitcoin/bitcoin/pull/24308#issuecomment-1037259386) for mingw builds.
The root of the problem is a broken implementation of [`std::filesystem::rename`](https://en.cppreference.com/w/cpp/filesystem/rename). In particular, the expected behavior
> If `old_p` is a non-directory file, then `new_p` must be ... existing non-directory file: `new_p` _is first deleted_...
fails with the "File exists" error.
This PR reverts back the `MoveFileExW` call, and adds the [suggested](https://github.com/bitcoin/bitcoin/pull/24308#pullrequestreview-878832906) unit test.
ACKs for top commit:
vasild:
ACK dc01cbc538765f64326bca30952c83e3862d0d54
Tree-SHA512: c8e5a98844cfa32bec0ad67a1aaa58fe2efd0c5474d3e83490211985b110f83245758a742dcaa0a933a192ab66a7f11807e0c53ae69260b7dd02fc99f6d03849
|
|
This change replaces repetitive code with a helper macro.
|
|
|
|
|
|
Use std::filesystem::rename() instead of std::rename(). We rely on the
destination to be overwritten if it exists, but std::rename()'s behavior
is implementation-defined in this case.
|
|
strencodings
fac9fe5d051264fcd16e8e36d30f28c05c999837 Fix unintended unsigned integer overflow in strencodings (MarcoFalke)
Pull request description:
This fixes two issues for strings that start with a colon and only have one colon:
* `fMultiColon` is incorrectly set to `true`
* There is an unsigned integer overflow `colon - 1` (`0 - 1`)
Neither issue matters, as the result is discarded. Though, it makes sense to still fix the issue for clarity and to avoid sanitizer issues in the function.
ACKs for top commit:
laanwj:
Code review ACK fac9fe5d051264fcd16e8e36d30f28c05c999837
shaavan:
Code Review ACK fac9fe5d051264fcd16e8e36d30f28c05c999837
Tree-SHA512: e71c21a0b617abf241e561ce6b90b963e2d5e2f77bd9547ce47209a1a94b454384391f86ef5d35fedd4f4df19add3896bb3d61fed396ebba8e864e3eeb75ed59
|
|
FormatParagraph
fa2f7d005932bff9b7d27744ae517b9e7910df8d fuzz: Avoid unsigned integer overflow in FormatParagraph (MarcoFalke)
Pull request description:
`FormatParagraph` is only ever called with compile time constant arguments, so I don't see the need for fuzzing it.
Though, keep it for now, but avoid the unsigned integer overflow with this patch.
ACKs for top commit:
laanwj:
Code review ACK fa2f7d005932bff9b7d27744ae517b9e7910df8d
Tree-SHA512: 01fc64a9ef73c183921ca1b0cd8db9514c0a242e3acf215a3393f383ae129e01625ebb16eaf9cb86370eda62d0145c3dcf8f62e40edf5958abc1f777c5687280
|
|
|
|
|
|
|
|
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
|
|
|
|
|
|
|
|
std::filesystem:::create_directories() call
b9c113af754540341d9529532fbadb7525168102 util: Avoid buggy std::filesystem:::create_directories() call (Hennadii Stepanov)
Pull request description:
Compiled with some libstdc++ versions (e.g., on Ubuntu 20.04) [`std::filesystem:::create_directories()`](https://en.cppreference.com/w/cpp/filesystem/create_directory) call [fails](https://github.com/bitcoin/bitcoin/issues/24257#issue-1123753243) to handle symbol links properly.
No behavior change in comparison to the [pre-20744](https://github.com/bitcoin/bitcoin/commit/c194293883fbb656779102309b2cb3e60889feff) master branch.
Fixes bitcoin/bitcoin#24257.
ACKs for top commit:
ryanofsky:
Code review ACK b9c113af754540341d9529532fbadb7525168102. Nice simplification and fix
MarcoFalke:
review ACK b9c113af754540341d9529532fbadb7525168102 🐬
Tree-SHA512: 79d940cfc1f68d9b0548fb2ab005e90850b54ac0fb3bb2940afd632d56288d92687579a3176bac3fd0ea3d2dae71e26444f8f7bdb87862414c12866ae5e857c4
|
|
Compiled with some libstdc++ versions (e.g., on Ubuntu 20.04)
std::filesystem:::create_directories() call fails to handle symbol links
properly.
|