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 77.4%. Comparing base (bf4a7b6) to head (3a5a707).
Report is 6 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5046     +/-   ##
=========================================
+ Coverage     76.2%   77.4%   +1.2%     
=========================================
  Files          760     762      +2     
  Lines        61568   64936   +3368     
  Branches      8126    8140     +14     
=========================================
+ Hits         46898   50249   +3351     
- Misses       14670   14687     +17

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

... and 576 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 · 2024-10-14T02:46:02Z

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

@Bronek I also fix it in the recent commit. Could you review as well?

Bronek

Good change, just some comments to improve unit tests.

Bronek · 2024-10-14T15:06:03Z

src/xrpld/rpc/handlers/AccountObjects.cpp

-            return RPC::invalid_field_error(jss::marker);
-
-        if (!entryIndex.parseHex(s))
+        if (!entryIndex.parseHex(markerStr.substr(idx + 1)))
            return RPC::invalid_field_error(jss::marker);
    }


really nice improvement in parsing here ! 👍

Bronek · 2024-10-14T15:11:27Z

src/test/rpc/AccountObjects_test.cpp

+            std::stringstream ss(marker.asString());
+            std::string s;
+            std::getline(ss, s, ',');
+            s = s + ",0";


can you pls also test when this gets corrupted not by ,0 but by a , alone ? i.e. valid dirIndex followed by a coma, then end of string. Also reverse, i.e. string starts with a coma, followed by valid entryIndex, then end of string. Reading up the implementation these both should return the same kind of error, but you need to have unit tests so that the code (which I think is good at this moment) does not degrade due to bad changes in the future.

Yes. In the recent commit, I added a lambda function to test invalid marker situations. And also added more invalid marker cases.

Bronek · 2024-10-14T15:14:55Z

src/test/rpc/AccountObjects_test.cpp

+        // test invalid marker by adding invalid string after the maker.
+        {
+            std::string s = marker.asString();
+            s.append(",1234");


I suggest replacing this test with a different one, where entryIndex is valid, see my other comment.

This is used to test adding some extra characters after the entryIndex. The marker format is like xxxx,yyyy,1234.

Nice, I missed it.

Bronek · 2024-10-16T11:36:03Z

@yinyiqian1 I allowed myself to merge the current develop to your branch, since it was presenting a confusing diff in API-CHANGELOG.md . Hope you do not mind.

Bronek

Good change, just minor remarks on unit tests.

Bronek · 2024-10-16T11:49:03Z

src/test/rpc/AccountObjects_test.cpp

+
+        env.close();
+
+        unsigned const limit = 11;


if you make it static constexpr ...

never mind, you can capture limit also without this change. Although constexpr would make it more obvious.

can I just update the lambda function capture to [&], you can review in my recent commit.

Bronek · 2024-10-16T11:49:31Z

src/test/rpc/AccountObjects_test.cpp

+        auto testInvalidMarker = [&env, &bob](std::string& marker) {
+            Json::Value params;
+            params[jss::account] = bob.human();
+            params[jss::limit] = 11;


... then you will be able to use limit here, without explicit capture.

Bronek · 2024-10-16T11:51:09Z

src/test/rpc/AccountObjects_test.cpp

+        auto const markerStr = marker.asString();
+        auto const& idx = markerStr.find(',');
+        auto const dirIndex = markerStr.substr(0, idx);
+        auto const entryIndex = markerStr.substr(idx + 1);


src/test/rpc/AccountObjects_test.cpp

Bronek · 2024-10-17T14:51:46Z

nice ! @yinyiqian1 do you agree this is ready to merge ?

yinyiqian1 · 2024-10-17T18:42:12Z

nice ! @yinyiqian1 do you agree this is ready to merge ?

@Bronek
previously I made a mistake in the lambda function, I queried "account_nfts" instead of "account_objects", I forced push into the previous commit to change it to "account_objects".

And in the most recent commit, when the marker is "0,entryIndex", and entryIndex is valid, it will succeed. Because when dirIndex=0, we will use root.key as dirIndex. Please check my most recent commit. Thank you.
Now I think it's ready to merge after the pipeline complete.

Bronek · 2024-10-18T13:51:31Z

@mDuo13 I think this PR does not require change in API-CHANGELOG.md because the erroneous behaviour being fixed here is not documented nor makes practical sense to use. Your opinion ?

mDuo13 · 2024-10-18T15:59:13Z

Yes, this sounds like a bug fix that does not require an API version change.

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

Modify the way parsing marker string

1bc0bc9

yinyiqian1 force-pushed the br_fix_account_obj_marker branch from 3e63249 to 1bc0bc9 Compare October 14, 2024 03:17

Bronek approved these changes Oct 14, 2024

View reviewed changes

Add more tests

141349c

yinyiqian1 force-pushed the br_fix_account_obj_marker branch from abb9d23 to 141349c Compare October 15, 2024 20:03

yinyiqian1 requested a review from Bronek October 15, 2024 20:05

Merge branch 'develop' into br_fix_account_obj_marker

d75eed3

Bronek approved these changes Oct 16, 2024

View reviewed changes

Bronek approved these changes Oct 17, 2024

View reviewed changes

update unit tests

66857f0

yinyiqian1 force-pushed the br_fix_account_obj_marker branch from 09f19ff to 66857f0 Compare October 17, 2024 18:26

update unit test

3a5a707

Bronek added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Oct 18, 2024

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

yinyiqian1 commented Oct 14, 2024

Bronek left a comment

Bronek Oct 14, 2024

Bronek Oct 14, 2024

yinyiqian1 Oct 15, 2024

Bronek Oct 14, 2024

yinyiqian1 Oct 15, 2024

Bronek Oct 16, 2024

Bronek commented Oct 16, 2024

Bronek left a comment

Bronek Oct 16, 2024

Bronek Oct 16, 2024

yinyiqian1 Oct 16, 2024

Bronek Oct 16, 2024

Bronek Oct 16, 2024

Bronek commented Oct 17, 2024

yinyiqian1 commented Oct 17, 2024 •

edited

Loading

Bronek commented Oct 18, 2024 •

edited

Loading

mDuo13 commented Oct 18, 2024

		// 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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 Oct 14, 2024

Bronek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bronek commented Oct 16, 2024

Bronek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bronek commented Oct 17, 2024

yinyiqian1 commented Oct 17, 2024 • edited Loading

Bronek commented Oct 18, 2024 • edited Loading

mDuo13 commented Oct 18, 2024

yinyiqian1 commented Jun 13, 2024 •

edited by Bronek

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

Bronek commented Oct 9, 2024 •

edited

Loading

yinyiqian1 commented Oct 17, 2024 •

edited

Loading

Bronek commented Oct 18, 2024 •

edited

Loading