/bitcoin

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Avoid returning a BIP9Stats object with uninitialized values #10957

Merged
merged 1 commit into from Aug 16, 2017
+1 −1

Conversation

Reviewers

@promag promag

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
8 participants
@practicalswift @sipa @pinheadmz @TheBlueMatt @jonasschnelli @MarcoFalke @fanquake @promag
@practicalswift
Contributor

practicalswift commented Jul 30, 2017 •
Edited 1 time

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 from Do not return a BIP9Stats object with uninitialized values to Avoid returning a BIP9Stats object with uninitialized values Jul 30, 2017

@sipa
Owner

sipa commented Jul 31, 2017

utACK bf2f4c3
@pinheadmz
Contributor

pinheadmz commented Jul 31, 2017

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.
src/versionbits.cpp
@@ -112,8 +112,12 @@ BIP9Stats AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockI
stats.period = Period(params);
stats.threshold = Threshold(params);
- if (pindex == NULL)
+ if (pindex == NULL) {
+ stats.elapsed = 0;
@promag

promag Jul 31, 2017

Contributor

I see no problem in:

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

TheBlueMatt commented Aug 1, 2017

Or maybe just initialize the stats object with an = {}?
@practicalswift
Avoid returning a BIP9Stats object with uninitialized values 
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;
}
```
3eb53b8
@practicalswift
Contributor

practicalswift commented Aug 1, 2017

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

@jonasschnelli jonasschnelli added the Refactoring label Aug 3, 2017

@jonasschnelli
Member

jonasschnelli commented Aug 3, 2017

utACK 3eb53b8
@MarcoFalke
Member

MarcoFalke commented Aug 3, 2017

utACK 3eb53b8
@TheBlueMatt
Contributor

TheBlueMatt commented Aug 3, 2017

utACK
@fanquake
Member

fanquake commented Aug 12, 2017

trivialACK 3eb53b8

@MarcoFalke MarcoFalke merged commit 3eb53b8 into bitcoin:master Aug 16, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Aug 16, 2017

@MarcoFalke
Merge #10957: Avoid returning a BIP9Stats object with uninitialized v… 
…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
This commit was signed with a verified signature.
@MarcoFalke MarcoFalke
GPG key ID: D2EA4850E7528B25 Learn about signing commits
a46a671

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Oct 3, 2017

@practicalswift @MarcoFalke
Avoid returning a BIP9Stats object with uninitialized values 
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: #10957
Rebased-From: 3eb53b8
50bd3f6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment