Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Software does not distinguish between different Conditions (Version: 1.5) #3417

Closed
cjcobb23 opened this issue May 28, 2020 · 4 comments
Closed

Software does not distinguish between different Conditions (Version: 1.5) #3417

cjcobb23 opened this issue May 28, 2020 · 4 comments
Assignees
@ckeshava
Labels
Bug

Comments

@cjcobb23
Copy link
Contributor

I discovered some code that just doesn't smell right. I don't know the intended behavior, but I suspect the intent is different than what is actually happening.

The code that checks whether the condition that is required for a given RPC is met does not actually consider what condition is required. All of the conditions except NO_CONDITION are treated exactly the same. All of the following lines (in the same function) evaluate to the exact same value (false if required_condition is NO_CONDITION and true otherwise).
https://github.com/ripple/rippled/blob/3d86b49dae8173344b39deb75e53170a9b6c5284/src/ripple/rpc/impl/Handler.h#L80
https://github.com/ripple/rippled/blob/3d86b49dae8173344b39deb75e53170a9b6c5284/src/ripple/rpc/impl/Handler.h#L90
https://github.com/ripple/rippled/blob/3d86b49dae8173344b39deb75e53170a9b6c5284/src/ripple/rpc/impl/Handler.h#L91
https://github.com/ripple/rippled/blob/3d86b49dae8173344b39deb75e53170a9b6c5284/src/ripple/rpc/impl/Handler.h#L97
https://github.com/ripple/rippled/blob/3d86b49dae8173344b39deb75e53170a9b6c5284/src/ripple/rpc/impl/Handler.h#L117
The reason these lines all evaluate to the exact same value is because of the way these conditions are defined: all except NO_CONDITION have the least significant bit set to 1. So, when we bitwise and any two conditions together, the result is false if either are NO_CONDITION and true otherwise. I don't think this was the intended behavior of the code, since all of these lines could be replaced with condition_required != NO_CONDITION. Maybe someone who understands the intention of this piece of code can chime in here.
@cjcobb23
Copy link
Contributor Author

For anyone scratching their head because git blame says I wrote this code, I actually just moved it from a different file. Prior to commit 7d867b806d70fc41fb45e3e61b719397033b272c, the code was here:
https://github.com/ripple/rippled/blob/761bb5744e9a43ebc6c94441edf8b2d8b1d662a4/src/ripple/rpc/impl/RPCHandler.cpp#L152

@nbougalis nbougalis added Bug Good First Issue Great issue for a new contributor labels May 28, 2020
@nbougalis
Copy link
Contributor

nbougalis commented May 28, 2020 •
edited
Loading

D'oh... this code is ancient (probably around 6 years old) and it does have the bug you describe, which is incredibly subtle.

I suspect the intention was to communicate that, e.g. NEEDS_CURRENT_LEDGER requires NEEDS_NETWORK_CONNECTION but all it did was make the code brittle. We should reconsider how this is handled, so that the condition checking code is both type-safe and robust by design.

In the meantime, your proposed solution is correct: replace the condition_required & WHATEVER statements with ((condition_required & WHATEVER) == WHATEVER). This retains the brittleness, but at least now we know about it.

Alternatively, we can change the enum values to be just powers of two, and change:

if ((condition_required & NEEDS_NETWORK_CONNECTION) &&
    (context.netOps.getOperatingMode() < OperatingMode::SYNCING))

to this:

// If any conditions are imposed, a network connection is required
if ((condition_required != NO_CONDITION) &&
    (context.netOps.getOperatingMode() < OperatingMode::SYNCING))

I think that this is minimal and probably less brittle.

@cjcobb23 cjcobb23 mentioned this issue Jun 15, 2020
Closed
This was referenced Jul 14, 2022
Closed
Merged
@Coderayush13
Copy link

Hey I want to work on these isssue

@intelliot
Copy link
Collaborator

@Coderayush13 or anyone else: please feel free to work on this issue. Comment here with any questions you have.

ckeshava pushed a commit to ckeshava/rippled that referenced this issue Jun 26, 2023
@ckeshava
fix: Update Handler::Condition enum values XRPLF#3417
248e2f0 
- Use powers of two to clearly indicate the bitmask
- Replace bitmask with explicit if-conditions to better indicate predicates
@intelliot intelliot removed Good First Issue Great issue for a new contributor Potential Bounty Idea labels Jun 27, 2023
intelliot added a commit that referenced this issue Jun 29, 2023
@intelliot
Revert "fix: Update Handler::Condition enum values #3417 (#4239)"
de85454 
This reverts commit 1c2ae10.
ximinez added a commit to ximinez/rippled that referenced this issue Jun 29, 2023
@ximinez
Merge remote-tracking branch 'upstream/develop' into github-ci
877f143 
* upstream/develop:
  ci: cancel overridden workflows (4597)
  fix: Update Handler::Condition enum values XRPLF#3417 (4239)
intelliot added a commit that referenced this issue Jun 30, 2023
@intelliot
Revert "fix: Update Handler::Condition enum values #3417 (#4239)"
c2250d3 
This reverts commit 1c2ae10.
ckeshava added a commit to ckeshava/rippled that referenced this issue Jul 10, 2023
@ckeshava
fix: Update Handler::Condition enum values XRPLF#3417 (XRPLF#4239)
a197d5b 
- Use powers of two to clearly indicate the bitmask
- Replace bitmask with explicit if-conditions to better indicate predicates

Change enum values to be powers of two (fix XRPLF#3417) XRPLF#4239

Implement the simplified condition evaluation
removes the complex bitwise and(&) operator
Implement the second proposed solution in Nik Bougalis's comment - Software does not distinguish between different Conditions (Version: 1.5) XRPLF#3417 (comment)
I have tested this code change by performing RPC calls with the commands server_info, server_state, peers and validation_info. These commands worked as expected.
ckeshava added a commit to ckeshava/rippled that referenced this issue Sep 22, 2023
@ckeshava
fix: Update Handler::Condition enum values XRPLF#3417 (XRPLF#4239)
e14d9c6 
- Use powers of two to clearly indicate the bitmask
- Replace bitmask with explicit if-conditions to better indicate predicates

Change enum values to be powers of two (fix XRPLF#3417) XRPLF#4239

Implement the simplified condition evaluation
removes the complex bitwise and(&) operator
Implement the second proposed solution in Nik Bougalis's comment - Software does not distinguish between different Conditions (Version: 1.5) XRPLF#3417 (comment)
I have tested this code change by performing RPC calls with the commands server_info, server_state, peers and validation_info. These commands worked as expected.
ckeshava added a commit to ckeshava/rippled that referenced this issue Sep 25, 2023
@ckeshava
fix: Update Handler::Condition enum values XRPLF#3417 (XRPLF#4239)
1d4a9c3 
- Use powers of two to clearly indicate the bitmask
- Replace bitmask with explicit if-conditions to better indicate predicates

Change enum values to be powers of two (fix XRPLF#3417) XRPLF#4239

Implement the simplified condition evaluation
removes the complex bitwise and(&) operator
Implement the second proposed solution in Nik Bougalis's comment - Software does not distinguish between different Conditions (Version: 1.5) XRPLF#3417 (comment)
I have tested this code change by performing RPC calls with the commands server_info, server_state, peers and validation_info. These commands worked as expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ckeshava ckeshava

Labels
Bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@intelliot @nbougalis @ckeshava @cjcobb23 @Coderayush13