fix account_objects with invalid marker do not return an error #5046

yinyiqian1 · 2024-06-13T03:16:58Z

fix #4542

High Level Overview of Change

Context of Change

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

codecov · 2024-06-18T22:02:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.2%. Comparing base (a753099) to head (029c474).
Report is 3 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5046     +/-   ##
=========================================
- Coverage     76.2%   76.2%   -0.0%     
=========================================
  Files          760     760             
  Lines        61568   61572      +4     
  Branches      8119    8122      +3     
=========================================
+ Hits         46909   46910      +1     
- Misses       14659   14662      +3

Files with missing lines	Coverage Δ
src/xrpld/rpc/detail/RPCHelpers.cpp	`82.2% <100.0%> (+0.4%)`	⬆️
src/xrpld/rpc/handlers/AccountObjects.cpp	`96.2% <100.0%> (+0.8%)`	⬆️

... and 4 files with indirect coverage changes

src/xrpld/rpc/detail/RPCHelpers.cpp

gregtatcam

👍 LGTM

Bronek

Thanks for fixing this bug. I have some comments which would hopefully make this fix a little better.

Bronek · 2024-10-09T12:27:28Z

src/test/rpc/AccountObjects_test.cpp

+            auto resp = env.rpc("json", "account_objects", to_string(params));
+            auto& accountObjects = resp[jss::result][jss::account_objects];
+            BEAST_EXPECT(!resp[jss::result].isMember(jss::error));
+            BEAST_EXPECT(


please also add here check that this is the last page:

BEAST_EXPECT(!resp[jss::result].isMember(jss::marker));

Bronek · 2024-10-09T12:29:59Z

src/xrpld/rpc/handlers/AccountObjects.cpp

+    // check if dirIndex is valid
+    if (!dirIndex.isZero() && !ledger->read({ltDIR_NODE, dirIndex}))
+        return RPC::invalid_field_error(jss::marker);


since RPC::getAccountObjects returning false would have the same result, I would move this check into the body of RPC::getAccountObjects; this will keep the checks (other than the marker format, in the code segment above) in that one function only, which is more robust.

Bronek · 2024-10-09T12:31:49Z

src/xrpld/rpc/detail/RPCHelpers.cpp

+        // non-zero dirIndex validity was checked in the caller function;
+        // by this point, it should be zero.


as commented elsewhere, I would prefer if this function did not rely on the validation of dirIndex happening in the caller, but rather move that check into the body of getAccountObjects

Bronek · 2024-10-09T12:35:52Z

While you are at it, I see that account_objects has pretty good unit test coverage but it could be still improved. See the red line 274 in https://app.codecov.io/gh/XRPLF/rippled/blob/develop/src%2Fxrpld%2Frpc%2Fhandlers%2FAccountObjects.cpp and red line 183 in https://app.codecov.io/gh/XRPLF/rippled/blob/develop/src%2Fxrpld%2Frpc%2Fdetail%2FRPCHelpers.cpp ; perhaps you could add tests for those two lines as well ?

EDIT: If you want to see coverage on your local computer, without waiting for codecov.io to catch up on your branch, see instructions in https://github.com/XRPLF/rippled/blob/develop/BUILD.md#coverage-report .

You will need Linux, gcc compiler and Python package gcovr; here's example (assuming you are in an empty "build" directory)

conan install .. --output-folder . --build missing --settings build_type=Debug
cmake -DCMAKE_BUILD_TYPE=Debug -Dcoverage=ON -Dxrpld=ON -Dtests=ON -Dcoverage_test_parallelism=1 -Dcoverage_format=html-details -Dcoverage_test="AccountObjects" -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake ..
cmake --build .
cmake --build . --target coverage

That will create coverage subdirectory with individual files matched to individual source files, and two root files index.html and index.css. The coverage in question is in files index.RPCHelpers.cpp.* and index.AccountObjects.cpp.* ; since the above supplied -Dcoverage_test="AccountObjects" this means only this one suite of tests was executed, which means the coverage will be generally very low but it still captured the tests we care for in this PR.

yinyiqian1 · 2024-10-10T17:22:45Z

While you are at it, I see that account_objects has pretty good unit test coverage but it could be still improved. See the red line 274 in https://app.codecov.io/gh/XRPLF/rippled/blob/develop/src%2Fxrpld%2Frpc%2Fhandlers%2FAccountObjects.cpp and red line 183 in https://app.codecov.io/gh/XRPLF/rippled/blob/develop/src%2Fxrpld%2Frpc%2Fdetail%2FRPCHelpers.cpp ; perhaps you could add tests for those two lines as well ?

Yes. I just added.

There might be a potential bug here, if we pass in some marker like xxxxxx,yyyyyy,,,,,,,,1234, suppose dirIndex and entryIndex part are valid, it will just ignore all the characters after the second ,. Do you think we should fix this as well?
Also just double check, the marker will always be two parts seperated by ,, right?

std::stringstream ss(marker.asString());
        std::string s;
        if (!std::getline(ss, s, ','))
            return RPC::invalid_field_error(jss::marker);

        if (!dirIndex.parseHex(s))
            return RPC::invalid_field_error(jss::marker);

        if (!std::getline(ss, s, ','))
            return RPC::invalid_field_error(jss::marker);

        if (!entryIndex.parseHex(s))
            return RPC::invalid_field_error(jss::marker);

Bronek · 2024-10-10T17:25:20Z

While you are at it, I see that account_objects has pretty good unit test coverage but it could be still improved. See the red line 274 in https://app.codecov.io/gh/XRPLF/rippled/blob/develop/src%2Fxrpld%2Frpc%2Fhandlers%2FAccountObjects.cpp and red line 183 in https://app.codecov.io/gh/XRPLF/rippled/blob/develop/src%2Fxrpld%2Frpc%2Fdetail%2FRPCHelpers.cpp ; perhaps you could add tests for those two lines as well ?

Yes. I just added.

There might be a potential bug here, if we pass in some marker like xxxxxx,yyyyyy,,,,,,,,1234, suppose dirIndex and entryIndex part are valid, it will just ignore all the characters after the second ,. Do you think we should fix this as well? Also just double check, the marker will always be two parts seperated by ,, right?
std::stringstream ss(marker.asString());
        std::string s;
        if (!std::getline(ss, s, ','))
            return RPC::invalid_field_error(jss::marker);

        if (!dirIndex.parseHex(s))
            return RPC::invalid_field_error(jss::marker);

        if (!std::getline(ss, s, ','))
            return RPC::invalid_field_error(jss::marker);

        if (!entryIndex.parseHex(s))
            return RPC::invalid_field_error(jss::marker);

It would be great to fix it, yes - @mDuo13 any comments ?

Bronek

nice !

yinyiqian1 marked this pull request as draft June 14, 2024 14:20

yinyiqian1 force-pushed the br_fix_account_obj_marker branch 2 times, most recently from f5edf6f to 6117405 Compare June 18, 2024 18:51

yinyiqian1 marked this pull request as ready for review June 18, 2024 19:16

gregtatcam requested review from gregtatcam and Bronek June 18, 2024 19:22

yinyiqian1 assigned yinyiqian1 and unassigned yinyiqian1 Jun 26, 2024

yinyiqian1 force-pushed the br_fix_account_obj_marker branch 3 times, most recently from 13b6555 to e66a096 Compare October 4, 2024 00:57

fix account_objects with invalid marker

627655e

yinyiqian1 force-pushed the br_fix_account_obj_marker branch from e66a096 to 627655e Compare October 4, 2024 02:08

gregtatcam reviewed Oct 7, 2024

View reviewed changes

src/xrpld/rpc/detail/RPCHelpers.cpp Show resolved Hide resolved

Add comment

fb37fc7

gregtatcam approved these changes Oct 9, 2024

View reviewed changes

Bronek reviewed Oct 9, 2024

View reviewed changes

add more tests

029c474

Bronek approved these changes Oct 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix account_objects with invalid marker do not return an error #5046

fix account_objects with invalid marker do not return an error #5046

yinyiqian1 commented Jun 13, 2024 •

edited by Bronek

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

gregtatcam left a comment

Bronek left a comment

Bronek Oct 9, 2024

yinyiqian1 Oct 10, 2024

Bronek Oct 9, 2024

yinyiqian1 Oct 10, 2024

Bronek Oct 9, 2024

yinyiqian1 Oct 10, 2024

Bronek commented Oct 9, 2024 •

edited

Loading

yinyiqian1 commented Oct 10, 2024

Bronek commented Oct 10, 2024

Bronek left a comment

		// non-zero dirIndex validity was checked in the caller function;
		// by this point, it should be zero.

fix account_objects with invalid marker do not return an error #5046

Are you sure you want to change the base?

fix account_objects with invalid marker do not return an error #5046

Conversation

yinyiqian1 commented Jun 13, 2024 • edited by Bronek Loading

High Level Overview of Change

Context of Change

Type of Change

API Impact

codecov bot commented Jun 18, 2024 • edited Loading

Codecov Report

gregtatcam left a comment

Choose a reason for hiding this comment

Bronek left a comment

Choose a reason for hiding this comment

Bronek Oct 9, 2024

Choose a reason for hiding this comment

yinyiqian1 Oct 10, 2024

Choose a reason for hiding this comment

Bronek Oct 9, 2024

Choose a reason for hiding this comment

yinyiqian1 Oct 10, 2024

Choose a reason for hiding this comment

Bronek Oct 9, 2024

Choose a reason for hiding this comment

yinyiqian1 Oct 10, 2024

Choose a reason for hiding this comment

Bronek commented Oct 9, 2024 • edited Loading

yinyiqian1 commented Oct 10, 2024

Bronek commented Oct 10, 2024

Bronek left a comment

Choose a reason for hiding this comment

yinyiqian1 commented Jun 13, 2024 •

edited by Bronek

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

Bronek commented Oct 9, 2024 •

edited

Loading