util: Fix Racy ParseOpCode function initialization #22875

JeremyRubin commented at 4:00 AM on September 3, 2021: contributor

If multiple callers call ParseOpCode concurrently it will cause a race condition. We can either move the static to it's own area and require init be called explicitly, or just allow concurrent first callers to race to fill in an atomic variable that never changes thereafter. The second approach is taken here.

Static initialization is threadsafe, but only insofar as definining the variable being guaranteed to be called once. This is used incorrectly here.

practicalswift --> are there tools we can deploy to catch usage like this?

fanquake deleted a comment on Sep 3, 2021

ajtowns commented at 9:15 AM on September 3, 2021: member

Wouldn't it be simpler to do something like:

class MapOpNames : public std::map<std::string, opcodetype>
{
public:
    MapOpNames()
    {
        for (unsigned int op = 0; op <= MAX_OPCODE; op++) {
            ...
        }
    }
};
opcodetype ParseOpCode(const std::string& s)
{
    const static MapOpNames mapOpNames;
    auto it = mapOpNames.find(s);
    ...
}

?

meshcollider requested review from practicalswift on Sep 3, 2021

kirillkovalenko commented at 11:19 AM on September 6, 2021: none

maybe just move map initialization out of ParseOpCode?

static std::map<std::string, opcodetype> get_map()
{
    std::map<std::string, opcodetype> map;
    for (unsigned int op = 0; op <= MAX_OPCODE; op++) {
          ...    
    }
    return map;
};

opcodetype ParseOpCode(const std::string& s)
{
    const static std::map<std::string, opcodetype> mapOpNames(get_map());
    auto it = mapOpNames.find(s);
    ...
}

MarcoFalke added the label Waiting for author on Sep 6, 2021

MarcoFalke added the label Refactoring on Sep 6, 2021

JeremyRubin commented at 8:09 PM on September 6, 2021: contributor

Those are both solid suggestions, I guess I wasn't familiar enough with initializer rules to see those would be safe. Will probably go with @kirillkovalenko's approach

JeremyRubin commented at 8:10 PM on September 6, 2021: contributor

Although thinking about it it's not clear to me that in your version get_map isn't called every time the function is run?

MarcoFalke commented at 7:23 AM on September 7, 2021: member

Why would it? You can test yourself:


#include <iostream>

static int get_map() {
  std::cout << __func__ << std::endl;
  return 42;
};

bool ParseOpCode() {
  const static auto mapOpNames{get_map()};
  return mapOpNames == 1;
}

int main() {
  ParseOpCode();
  ParseOpCode();
}

kirillkovalenko commented at 8:18 AM on September 7, 2021: none

Although thinking about it it's not clear to me that in your version get_map isn't called every time the function is run?

It's the way language/runtime implements it, basically a guarantee https://en.cppreference.com/w/cpp/language/storage_duration#Static_local_variables

JeremyRubin commented at 4:50 PM on September 7, 2021: contributor

It doesn't seem to me clear that the behavior is specified (although certainly it seems to be defined) to be not equivalent to something like this:

static int v = f();

int&& x = f();
static int v = x;

Whereby f() is always invoked since v is still only initialized once in either case.

AJ's version does not have this ambiguity (but I'd likely rather make it composition than inheritance as inherit from std is generally a smell).

MarcoFalke commented at 6:20 AM on September 8, 2021: member

There are issues in other parts of the codebase if this assumption doesn't hold

JeremyRubin commented at 3:58 PM on September 8, 2021: contributor

reading the spec more closely I think it should be ok

ajtowns commented at 10:13 AM on September 9, 2021: member

I think you could also do:

namespace {
class OpCodeParser
{
private:
    std::map<std::string, opcodetype> mapOpNames;

public:
    OpCodeParser()
    {
        for (unsigned int op = 0; op <= MAX_OPCODE; op++) {
            ...
        }
    }
    opcodetype Parse(const std::string& s) const {
        auto it = mapOpNames.find(s);
        ...
    }
};    
} // namespace

opcodetype ParseOpCode(const std::string& s)
{
    const static OpCodeParser ocp;
    return ocp.Parse(s);
}

JeremyRubin force-pushed on Sep 10, 2021

JeremyRubin commented at 9:03 PM on September 10, 2021: contributor

went with @ajtowns suggested edit.

JeremyRubin force-pushed on Sep 10, 2021

in src/core_read.cpp:21 in d5e006c84a outdated

  17 | @@ -18,18 +18,19 @@
  18 |  #include <boost/algorithm/string/split.hpp>
  19 |  
  20 |  #include <algorithm>
  21 | +#include <atomic>

luke-jr commented at 2:46 AM on September 26, 2021:

Don't think this is needed anymore?

in src/core_read.cpp:33 in d5e006c84a outdated

  34 | +public:
  35 | +    OpCodeParser()
  36 |      {
  37 | -        for (unsigned int op = 0; op <= MAX_OPCODE; op++)
  38 | -        {
  39 | +        for (unsigned int op = 0; op <= MAX_OPCODE; op++) {

luke-jr commented at 2:47 AM on September 26, 2021:

nit

        for (unsigned int op = 0; op <= MAX_OPCODE; ++op) {

kirillkovalenko commented at 9:24 PM on September 27, 2021:

Does it have any impact on codegen? I thought it's not longer the case since like 2002.

luke-jr commented at 9:42 PM on September 27, 2021:

That's why it's just a nit. "The compiler can fix it" isn't really a good reason to keep poor code.

laanwj commented at 4:04 PM on November 26, 2021:

Does it have any impact on codegen? I thought it's not longer the case since like 2002.

Not in the case of an integer. Though in general it's better to write ++x in for loops even if just to avoid these kind of code review comments :smile:

in src/core_read.cpp:53 in d5e006c84a outdated

  44 | @@ -44,10 +45,17 @@ opcodetype ParseOpCode(const std::string& s)
  45 |              }
  46 |          }
  47 |      }
  48 | +    opcodetype Parse(const std::string& s) const {
  49 | +        auto it = mapOpNames.find(s);
  50 | +        if (it == mapOpNames.end()) throw std::runtime_error("script parse error: unknown opcode");
  51 | +        return it->second;

luke-jr commented at 2:48 AM on September 26, 2021:

        return mapOpNames.at(s);

JeremyRubin commented at 6:11 PM on September 27, 2021:

NACK; this is a change to the error that gets returned.

MarcoFalke removed the label Waiting for author on Sep 26, 2021

in src/core_read.cpp:50 in d5e006c84a outdated

  44 | @@ -44,10 +45,17 @@ opcodetype ParseOpCode(const std::string& s)
  45 |              }
  46 |          }
  47 |      }
  48 | +    opcodetype Parse(const std::string& s) const {

MarcoFalke commented at 7:03 AM on September 28, 2021:

nit: according to clang-format:

diff --git a/src/core_read.cpp b/src/core_read.cpp
index bd29e2073e..ba535f745f 100644
--- a/src/core_read.cpp
+++ b/src/core_read.cpp
@@ -45,7 +45,8 @@ public:
             }
         }
     }
-    opcodetype Parse(const std::string& s) const {
+    opcodetype Parse(const std::string& s) const
+    {
         auto it = mapOpNames.find(s);
         if (it == mapOpNames.end()) throw std::runtime_error("script parse error: unknown opcode");
         return it->second;

sipa commented at 10:04 PM on December 17, 2021:

Nit coding-style: opening brace for functions/classes on a separate line.

DrahtBot commented at 11:42 PM on October 8, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

luke-jr referenced this in commit 8009a9ba4c on Oct 10, 2021

DrahtBot added the label Needs rebase on Oct 12, 2021

MarcoFalke commented at 11:57 AM on October 22, 2021: member

Are you still working on this? If not, I suggest to close this PR

MarcoFalke commented at 11:58 AM on October 22, 2021: member

practicalswift --> are there tools we can deploy to catch usage like this?

bitcoin-tx is single threaded. I don't think tooling exists to catch "races" in single-threaded programs, whatever that means.

JeremyRubin commented at 4:56 PM on October 22, 2021: contributor

well i think core_read is still included in libbitcoin sources so i think it should be threadsafe -- good rule of thumb is that any pure function should be threadsafe, always.

i don't think closing the PR is a good idea; i'll try to rebase it when i have time.

laanwj commented at 4:07 PM on November 26, 2021: member

good rule of thumb is that any pure function should be threadsafe, always.

There's some risk that this function gets moved to an utility at some point when it is needed somewhere else, and people forget to make it thread-safe.

I'm fine with making this change, but please address the comments.

luke-jr referenced this in commit 49563bbbe2 on Dec 14, 2021

JeremyRubin force-pushed on Dec 17, 2021

DrahtBot removed the label Needs rebase on Dec 17, 2021

JeremyRubin force-pushed on Dec 17, 2021

JeremyRubin commented at 9:47 PM on December 17, 2021: contributor

rebased, nits addressed

in src/core_read.cpp:33 in cfa2b8dbf9 outdated

  33 | -        for (unsigned int op = 0; op <= MAX_OPCODE; op++) {
  34 | +public:
  35 | +    OpCodeParser()
  36 | +    {
  37 | +        for (unsigned int op = 0; op <= MAX_OPCODE; ++op)
  38 | +        {

sipa commented at 10:03 PM on December 17, 2021:

Nit: coding style. Opening brace for anything but classes/functions is on the same line.

sipa commented at 10:05 PM on December 17, 2021: member

Code review ACK cfa2b8dbf9fc6d020281e6521e83853838f62f81

MarcoFalke commented at 10:44 AM on December 22, 2021: member

review ACK cfa2b8dbf9fc6d020281e6521e83853838f62f81 🗡

<details><summary>Show signature</summary>

Signature:

</details>

MarcoFalke commented at 10:45 AM on December 22, 2021: member

@JeremyRubin would be really nice to fix the two nits, but this can be also be merged as-is. Please let us know what you prefer.

JeremyRubin commented at 7:48 AM on December 23, 2021: contributor

since the current PR has 2 code review ACKs on cfa2b8d, i don't want to burden anyone with re-review on that hash, so I pushed a fixup with it. If you want i can squash it, or you can on merge.

MarcoFalke commented at 7:57 AM on December 23, 2021: member

Thank you for the fixup. Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

MarcoFalke commented at 7:58 AM on December 23, 2021: member

maintainers aren't allowed to squash, as it would break the signature check script

MarcoFalke renamed this:
~~Fix Racy ParseOpCode function initialization~~
util: Fix Racy ParseOpCode function initialization
on Dec 23, 2021

MarcoFalke added the label Utils/log/libs on Dec 23, 2021

Fix Racy ParseOpCode function initialization 7b481f015a

JeremyRubin force-pushed on Dec 23, 2021

JeremyRubin commented at 11:54 PM on December 23, 2021: contributor

squashed

MarcoFalke approved

MarcoFalke commented at 9:00 AM on December 24, 2021: member

re-ACK 7b481f015a0386f5ee3e13415474952653ddc198 🗣

<details><summary>Show signature</summary>

Signature:

</details>

MarcoFalke merged this on Dec 24, 2021

MarcoFalke closed this on Dec 24, 2021

sidhujag referenced this in commit 174e187c75 on Dec 27, 2021

DrahtBot locked this on Dec 24, 2022