lambda function in src/script/miniscript.h:571: asserts() on invalid input script (if compiled in debug mode), but does not return a valid string otherwise #25904

1. 
   verdy-p commented at 3:28 pm on August 22, 2022: none

   Some possibly invalid input miniscripts may cause the miniscript evaluator to misbehave:

   • when compiling in debug mode, at least we get an assertion failure (the program will crash and exit).
   • when compiling in the release, that function may return some random std::string reference, causing UB.

   See the lamda fucntion starting on src/script/miniscript.h:571.

   Detected by gcc during compilation with -Wreturn-type. But as it is in a lambda, itself hidden in a template, it was hard to see at the location indicated (which is just the 1st line of the lambda missing some valid return).

   So the final assert(false); is insufficient, it should at least be an exception that can be caught, or some valid string such as "" (depending on the expected semantics).

2. verdy-p added the label Bug on Aug 22, 2022
3. verdy-p renamed this:
   lambda function in src/script/miniscript.h:571: asserts() on invalid input script (if compiled ni debug mode), but does not return a valid string otherwise
   lambda function in src/script/miniscript.h:571: asserts() on invalid input script (if compiled in debug mode), but does not return a valid string otherwise
   on Aug 22, 2022
4. 
   MarcoFalke commented at 3:31 pm on August 22, 2022: member

   Is this still an issue with a recent version of Bitcoin Core? If yes, what are the steps to reproduce?
5. 
   sipa commented at 3:31 pm on August 22, 2022: member

   Which fragment type is not handled? If there is a missing one, it should assert.
6. 
   verdy-p commented at 3:33 pm on August 22, 2022: none

   Any fragment type that does not match one of the handled values (those in the cases of the switch). I think there may be invalid types in some transactions from incoming blocks and containing miniscripts. This explains the warning you get when compiling. I don’t know have there may be other Fragment::TYPE values, but this may come through some indirect references not passing through legitimate constructors. I’ve added a return "" after the assert(false) and this seems to fix some issues in the indexer caused by some new incoming blocks: may be the uncoming miniscripts are not properly validated ansd assumed to be valid (I’ve heard a recent proposal of extension for validation miniscripts to return at least a zero. May be some other nodes are experimenting with new opcodes.
7. 
   sipa commented at 3:34 pm on August 22, 2022: member

   Both switch statements in the function combined should handle all fragment types. If you have a counter example, I’m happy to hear it.
8. 
   sipa commented at 3:38 pm on August 22, 2022: member

   Also, Bitcoin Core cannot be compiled without assertions, see src/util/check.h.
9. 
   MarcoFalke commented at 3:40 pm on August 22, 2022: member

   Are you reporting a false positive compile warning? We need exact steps to reproduce to investigate. Otherwise I am going to close this thread for now.
10. 
    sipa commented at 3:42 pm on August 22, 2022: member

    Lastly, the miniscript code is only used by the wallet, it is not run on transactions or blocks received from the network.
11. 
    verdy-p commented at 3:45 pm on August 22, 2022: none

    This is a true positive warning from the compiler: assert() is not warrantied to never return (even with what is in src/util/check.h, or the compielr does not know it and generates an incorrect lambda for that case, possibly wrapping the expected result type of the lamnda into another type)

    Then we get the issue when calling the constructor for TreeEvalMaybe().

    Now the wallet constent is checked with any incoming block: could this explain the issue when indexing the coinsdb, which prepares the chain for efficient detection of submitted/validated transactions coming from the wallet?

    Also what does happen when there’s no wallet open?

    (And sorry if this looks like noise, but I’m discovering the source code and there are a lot of things that I need to understand). But for me a lamdbda must ALWAYS return a valid value, compatible with its declared type and not assume a behavior from assert() inside it. I think this is a case of UB in C++17.

12. 
    sipa commented at 3:51 pm on August 22, 2022: member

    Then we get the issue when calling the constructor for TreeEvalMaybe().

    What issue?

    Now the wallet constent is checked with any incoming block: could this explain the issue when indexing the coinsdb, which prepares the chain for efficient detection of submitted/validated transactions coming from the wallet?

    That seems extremely unlikely. The miniscript code is not involved in block/transaction processing at all; it is only used by the wallet to populate its list of addresses/scripts to look for. And it is only used when you’ve actually explicitly imported a miniscript descriptor - which is a new feature in the current master codebase (doesn’t exist in any release).

    Also what does happen when there’s no wallet open?

    Then by definition you can’t have a wallet with miniscript descriptors open, and it’s even more obvious this code isn’t executed.

    But for me a lamdbda must ALWAYS return a valid value,

    It is. The assert is in unreachable code.

13. 
    verdy-p commented at 3:52 pm on August 22, 2022: none

    But for me a lamdbda must ALWAYS return a valid value,

    It is. The assert is in unreachable code.

    But gcc does not detect it and warns, it probably does not know what to do in that case for compiling the lambda correctly. If it assumes that the end of cuntion is reachable, it may incorrectly place the wrapper code for exception handlers or garbage collectors and reference counting.

14. 
    MarcoFalke commented at 3:55 pm on August 22, 2022: member

    gcc has known issues around that. You can try a more modern compiler. In any case, without steps to reproduce and the compiler version you use, there is nothing we can do.
15. 
    sipa commented at 3:57 pm on August 22, 2022: member

    But gcc does not detect it and warns, it probably does not know what to do in that case for compiling the lambda correctly.

    I don’t see such a warning (GCC 11.2.0). What version are you using? What are the steps to reproduce?

    We can consider adding a return statement here, but that’s effectively silencing the unreachable code warning - which would otherwise be able to tell us whether there are actually missing case statements.

    it may incorrectly place the wrapper code for exception handlers or garbage collectors and reference counting.

    That’s not relevant unless there is a way to actually reach that code. So far I have not seen any evidence that that is possible. If a counterexample exist, it should be trivial to find - either by hand, or by our fuzzers which have been run extensively on this code.

16. 
    verdy-p commented at 4:02 pm on August 22, 2022: none

    Do you speak about src/wallet/test/fuzz/test_test.cpp ? it does not compile correctly (undefined reference to `__assert_fail’ in “../include/minisketch.h”), with HOST=x86_64-w64-mingw32
17. 
    sipa commented at 4:05 pm on August 22, 2022: member

    src/test/fuzz/miniscript.cpp tests lots of miniscript aspects, including the ToString(). It has been run by many people, including Google’s oss-fuzz large-scale fuzzing infrastructure for open source code.
18. 
    verdy-p commented at 4:06 pm on August 22, 2022: none

    So this is a gcc bug? Using gcc (Debian 10.2.1-6) 10.2.1 20210110; the default version in Debian Bullseye; possibly there’s a patch to apply or a newer version to get from some other deb/apt package repository. If the end of the lambda was really unreachable, I would get a “unreachable code” warning for the additional return ""; statement I added just after assert(false); (only in a try to fix it and see the result)
19. 
    fanquake commented at 4:16 pm on August 22, 2022: member

    it does not compile correctly (undefined reference to `__assert_fail’ in “../include/minisketch.h”), with HOST=x86_64-w64-mingw32

    Unless you provide the actual commands you used to reproduce this issue there is very little we can do here. I’ve tested that cross-compiling for windows on Debian Bullseye using x86_64-w64-mingw32-g++ (GCC) 10-posix 20210110 works as expected.

20. 
    verdy-p commented at 4:22 pm on August 22, 2022: none

    I just used the statements given in doc/build-windows.md:

    First in bash, export HOST=x86_64-w64-mingw32

    Then all the steps:

    0    PATH=$(echo "$PATH" | sed -e 's/:\/mnt.*//g') # strip out problematic Windows %PATH% imported var
    1    sudo bash -c "echo 0 > /proc/sys/fs/binfmt_misc/status" # Disable WSL support for Win32 applications.
    2    cd depends
    3    make HOST=x86_64-w64-mingw32
    4    cd ..
    5    ./autogen.sh
    6    CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site ./configure --prefix=/
    

    (no issue; it jsut takes long to compile Qt libraries; I should have disabled it because I don’t intend to use the Qt binary, just the daemon and the CLI. Also I’ve not experienced with the “‘multiprocess” option which uses IPC rather than builtin RPC).

    Then just make clean; make -j7 and I see these warnings. and finally fuzz does not compile. So I had to disable fuzz tests for Windows64.

    Note that for compiling the Debian version, I just run ‘./configure’ then ‘make clean; make -j7’ and fuzz does compile.

    I do not compile with -g (debug), it is considerably slower at run time.

21. 
    verdy-p commented at 4:26 pm on August 22, 2022: none

    You may think I am extremely unlucky, or that there are things to do not stated in the docs. I do not progress much since long with Bitcoin Core and see issues all the time on all platforms. It’s hard then to experiment something else as long as I don’t find any stable environment.
22. MarcoFalke removed the label Bug on Aug 23, 2022
23. MarcoFalke added the label Upstream on Aug 23, 2022
24. 
    MarcoFalke commented at 5:55 am on August 23, 2022: member

    Ok, so you are cross compiling with mingw (the gcc port), not vanilla gcc.

    As I mentioned earlier, the false positive warnings on gcc are known, see https://cirrus-ci.com/task/6213872875995136?logs=ci#L3690 :

     0...
     1  CXX      test/fuzz/fuzz-integer.o
     2  CXX      test/fuzz/fuzz-key.o
     3  CXX      test/fuzz/fuzz-key_io.o
     4  CXX      test/fuzz/fuzz-kitchen_sink.o
     5  CXX      test/fuzz/fuzz-load_external_block_file.o
     6  CXX      test/fuzz/fuzz-locale.o
     7  CXX      test/fuzz/fuzz-merkleblock.o
     8  CXX      test/fuzz/fuzz-message.o
     9  CXX      test/fuzz/fuzz-miniscript.o
    10  CXX      test/fuzz/fuzz-minisketch.o
    11In file included from test/fuzz/miniscript.cpp:8:
    12./script/miniscript.h: In lambda function:
    13./script/miniscript.h:571:25: warning: control reaches end of non-void function [-Wreturn-type]
    14  571 |             std::string ret = wrapped ? ":" : "";
    15      |                         ^~~
    16  CXX      test/fuzz/fuzz-muhash.o
    17  CXX      test/fuzz/fuzz-multiplication_overflow.o
    18  CXX      test/fuzz/fuzz-net.o
    19...
    

25. 
    MarcoFalke commented at 5:56 am on August 23, 2022: member

    Let us know if you have any other questions
26. MarcoFalke closed this on Aug 23, 2022

    ---

27. MarcoFalke added the label Questions and Help on Aug 23, 2022
28. 
    verdy-p commented at 9:14 pm on August 23, 2022: none

    I used mingw because that is what you document and support for building the Windows version. It was done on the same OS (Debian Bullseye) on which I have also compiled the Linux version (the only difference being the HOST=* parameter for specifying a target.

    I have not tried Clang instead of gcc, jsut used the default compiler automatically selected by your existing documented settings. so I also did not try to compile the Windows version with Visual Studio (I’ve tried it once, and found no easy way to configure it correctly… too many dependencies, and Visual Studio is quite unstable in its own requirements, and updates too often, and like any compiler could have its own bugs at any time, that will be hard to reproduce by someone else, especuialyl when working with the master development git sources which change some details every few days).

    May be I should stick on using a stable branch 23.x instead of the latest head with “git pull”, but I’m not sure that it gets updated with backports for all possible bug resolutions rapidly tested in the master dev branch.

    I’m still looking for causes of bugs/hangs that exist and persist in the Windows released versions 22.0 or 23.0 trying to isolate them. It’s not easy, and won’t be easy if you also don’t support “mingw” as a “vanilla” environment.

    So which environment or settings do you recommend for testing/debugging the Windows version? Do you even support Windows?

    Or do you support just plain Linux with LTS releases (also working well in stable and supported hypervisors or on wellknown hosting providers), such as RHEL8, AlmaLinux, Oracle Linux, Debian, Ubuntu, or possibly PopOS and Mint…

    But probably not Fedora, not CentOS Stream (replacing CentOS which is no longer supported, but “CentOS Stream” is no longer “stable”, looking much like a rolling release even if it is a bit slower and more tested), and certainly not popular distribs using “rolling releases” (which no one should use for BitCoin Core), and not “custom” kernels tuned for specific hardware and specific apps (like Gentoo, which may be extremely stable but too highly personalized with highly variable dependencies that are difficult and very lengthy to configure and test, even if there are Gentoo-based or ArchLinux-based distribs offering precompiled binaries: these special “custom” distribs require a very solid PC, lot of RAM, fast storage, a fast CPU with many cores, so they are only for some costly “high-en” PC’s used by “geekies” at home that are looking for tweaks to get extreme performances in specific apps, or used in professional developement labs for designing new systems with months of development and solid teams).

    I don’t say that Bitcoin Core will not work on these distribs but there’s no warranty, unless their users have very good technical knowledge and experience of their system (and have enough time to invest on them and ara capable to reproduce and isolate their problems, something that most users will not be able to do). Even me, I have not used C++ extensively since long to follow how its “standard” evolved. My C++ skills are still very classic and today I no longer use it for most things (I prefer using C most of the time, as it is much more “stable” and ahve little “magic” and new subtle semantics to hide). Most of the changes made in C++ are in fact already reflected in other languages (like Python, PHP, Java…) whose performance is now largely sufficient without taking many security risks difficult to solve. I’ev seen that you are now experimenting with C++20, but as now you use C++17, you are far more advanced than me about it.

    The problem is that you may have difficulty to recruit more testers and to provide guiding for candidates: your code is really very complex (and I find Bitcoin Core sources even more complex to understand than the Linux kernel itself, even if it’s much smaller; even the sources for Qt is simpler for me to work with, as it is more modular and has much more documentation, and lot of online help if needed).

    But I have noted your current efforts to modularize your sources (thanks for that), and possibly develop your existing support with more volunteers.

29. 
    hebasto commented at 9:45 pm on August 23, 2022: member

    So this is a gcc bug?

    https://sourceforge.net/p/mingw-w64/bugs/306/

30. 
    MarcoFalke commented at 5:54 am on August 24, 2022: member

    So which environment or settings do you recommend for testing/debugging the Windows version? Do you even support Windows?

    Yes, Windows is supported. The false positive compiler warning is a false positive and thus it can be ignored

31. MarcoFalke reopened this on Aug 24, 2022

    ---

32. MarcoFalke closed this on Aug 24, 2022

    ---

33. bitcoin locked this on Aug 24, 2023

Contributors
verdy-p MarcoFalke sipa fanquake hebasto

Labels
Upstream Questions and Help