Skip to content

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jul 30, 2017

Uninitialized data potentially used in rpc/blockchain.cpp:

static UniValue BIP9SoftForkDesc(const Consensus::Params& consensusParams, Consensus::DeploymentPos id)
{
    ...
    const ThresholdState thresholdState = VersionBitsTipState(consensusParams, id);
    ...
    if (THRESHOLD_STARTED == thresholdState)
    {
        UniValue statsUV(UniValue::VOBJ);
        BIP9Stats statsStruct = VersionBitsTipStatistics(consensusParams, id);
        statsUV.push_back(Pair("period", statsStruct.period));
        statsUV.push_back(Pair("threshold", statsStruct.threshold));
        statsUV.push_back(Pair("elapsed", statsStruct.elapsed));
        statsUV.push_back(Pair("count", statsStruct.count));
        statsUV.push_back(Pair("possible", statsStruct.possible));
        rv.push_back(Pair("statistics", statsUV));
    }
    ...
    return rv;
}

Friendly ping @pinheadmz :-)

@practicalswift practicalswift changed the title Do not return a BIP9Stats object with uninitialized values Avoid returning a BIP9Stats object with uninitialized values Jul 30, 2017
@sipa
Copy link
Member

sipa commented Jul 31, 2017

utACK bf2f4c36c8e37a29f81833505f76a99c5faa8a79

@pinheadmz
Copy link
Member

utACK thanks! I think this would only apply to the genesis block, right? Which would never be THRESHOLD_STARTED... I don't know if that pindex==NULL code will ever run actually.

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK bf2f4c3.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no problem in:

  • moving all default values to BIP9Stats constructor, or
  • moving these up even if they are overwritten below.

@TheBlueMatt
Copy link
Contributor

Or maybe just initialize the stats object with an = {}?

Uninitialized data potentially used in `rpc/blockchain.cpp`:

```
static UniValue BIP9SoftForkDesc(const Consensus::Params& consensusParams, Consensus::DeploymentPos id)
{
    ...
    const ThresholdState thresholdState = VersionBitsTipState(consensusParams, id);
    ...
    if (THRESHOLD_STARTED == thresholdState)
    {
        UniValue statsUV(UniValue::VOBJ);
        BIP9Stats statsStruct = VersionBitsTipStatistics(consensusParams, id);
        statsUV.push_back(Pair("period", statsStruct.period));
        statsUV.push_back(Pair("threshold", statsStruct.threshold));
        statsUV.push_back(Pair("elapsed", statsStruct.elapsed));
        statsUV.push_back(Pair("count", statsStruct.count));
        statsUV.push_back(Pair("possible", statsStruct.possible));
        rv.push_back(Pair("statistics", statsUV));
    }
    ...
    return rv;
}
```
@practicalswift
Copy link
Contributor Author

@TheBlueMatt Thanks for the review! Fixed! :-)

@jonasschnelli
Copy link
Contributor

utACK 3eb53b8

@maflcko
Copy link
Member

maflcko commented Aug 3, 2017

utACK 3eb53b8

@TheBlueMatt
Copy link
Contributor

utACK

@fanquake
Copy link
Member

trivialACK 3eb53b8

@maflcko maflcko merged commit 3eb53b8 into bitcoin:master Aug 16, 2017
maflcko pushed a commit that referenced this pull request Aug 16, 2017
…alues

3eb53b8 Avoid returning a BIP9Stats object with uninitialized values (practicalswift)

Pull request description:

  Uninitialized data potentially used in `rpc/blockchain.cpp`:

  ```
  static UniValue BIP9SoftForkDesc(const Consensus::Params& consensusParams, Consensus::DeploymentPos id)
  {
      ...
      const ThresholdState thresholdState = VersionBitsTipState(consensusParams, id);
      ...
      if (THRESHOLD_STARTED == thresholdState)
      {
          UniValue statsUV(UniValue::VOBJ);
          BIP9Stats statsStruct = VersionBitsTipStatistics(consensusParams, id);
          statsUV.push_back(Pair("period", statsStruct.period));
          statsUV.push_back(Pair("threshold", statsStruct.threshold));
          statsUV.push_back(Pair("elapsed", statsStruct.elapsed));
          statsUV.push_back(Pair("count", statsStruct.count));
          statsUV.push_back(Pair("possible", statsStruct.possible));
          rv.push_back(Pair("statistics", statsUV));
      }
      ...
      return rv;
  }
  ```

  Friendly ping @pinheadmz :-)

Tree-SHA512: cc1debe11d81157b9fa8e6064bfec199524cd1e2d0230ff35f45d97ecabbc664df8423edb1c9e4ba3daf19bbd51ab87bb50e5e5cd279be1d2aa1f7d8b300f148
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 3, 2017
Uninitialized data potentially used in `rpc/blockchain.cpp`:

```
static UniValue BIP9SoftForkDesc(const Consensus::Params& consensusParams, Consensus::DeploymentPos id)
{
    ...
    const ThresholdState thresholdState = VersionBitsTipState(consensusParams, id);
    ...
    if (THRESHOLD_STARTED == thresholdState)
    {
        UniValue statsUV(UniValue::VOBJ);
        BIP9Stats statsStruct = VersionBitsTipStatistics(consensusParams, id);
        statsUV.push_back(Pair("period", statsStruct.period));
        statsUV.push_back(Pair("threshold", statsStruct.threshold));
        statsUV.push_back(Pair("elapsed", statsStruct.elapsed));
        statsUV.push_back(Pair("count", statsStruct.count));
        statsUV.push_back(Pair("possible", statsStruct.possible));
        rv.push_back(Pair("statistics", statsUV));
    }
    ...
    return rv;
}
```

Github-Pull: bitcoin#10957
Rebased-From: 3eb53b8
codablock pushed a commit to codablock/dash that referenced this pull request Sep 25, 2019
…lized values

3eb53b8 Avoid returning a BIP9Stats object with uninitialized values (practicalswift)

Pull request description:

  Uninitialized data potentially used in `rpc/blockchain.cpp`:

  ```
  static UniValue BIP9SoftForkDesc(const Consensus::Params& consensusParams, Consensus::DeploymentPos id)
  {
      ...
      const ThresholdState thresholdState = VersionBitsTipState(consensusParams, id);
      ...
      if (THRESHOLD_STARTED == thresholdState)
      {
          UniValue statsUV(UniValue::VOBJ);
          BIP9Stats statsStruct = VersionBitsTipStatistics(consensusParams, id);
          statsUV.push_back(Pair("period", statsStruct.period));
          statsUV.push_back(Pair("threshold", statsStruct.threshold));
          statsUV.push_back(Pair("elapsed", statsStruct.elapsed));
          statsUV.push_back(Pair("count", statsStruct.count));
          statsUV.push_back(Pair("possible", statsStruct.possible));
          rv.push_back(Pair("statistics", statsUV));
      }
      ...
      return rv;
  }
  ```

  Friendly ping @pinheadmz :-)

Tree-SHA512: cc1debe11d81157b9fa8e6064bfec199524cd1e2d0230ff35f45d97ecabbc664df8423edb1c9e4ba3daf19bbd51ab87bb50e5e5cd279be1d2aa1f7d8b300f148
@practicalswift practicalswift deleted the bip9status branch April 10, 2021 19:32
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants