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

test: Check for some unlikely null dereferences in tests #5004

Closed
wants to merge 38 commits into from
Closed

test: Check for some unlikely null dereferences in tests #5004

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
95ca921
test: Check for some unlikely null dereferences in tests
ximinez Apr 26, 2024
7ae906c
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Apr 26, 2024
540713d
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez May 1, 2024
a926a2d
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez May 7, 2024
d5dd37b
Merge commit 'c706926' into pr/test_fixes
ximinez Jul 2, 2024
546cd42
Merge commit 'f6879da' into pr/test_fixes
ximinez Jul 2, 2024
16835cf
Move CMake directory
Jul 2, 2024
0ff9758
Rearrange sources
Jul 2, 2024
b52276e
Rewrite includes
Jul 2, 2024
501f1f8
Recompute loops
Jul 2, 2024
fb8b17e
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Jul 2, 2024
85beb42
Fix formatting
ximinez Jul 2, 2024
a173ecc
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Jul 5, 2024
38344ba
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Jul 10, 2024
1c9d1bb
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Jul 19, 2024
045e50f
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Jul 24, 2024
d51702d
[FOLD] Review feedback from @ckeshava:
ximinez Jul 25, 2024
0c867c9
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Jul 25, 2024
828c444
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Jul 29, 2024
46ecfe2
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Aug 2, 2024
5508609
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Aug 6, 2024
9a8cb65
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Aug 8, 2024
24aad0a
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Aug 19, 2024
2c00a3e
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Sep 5, 2024
ef96f58
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Sep 11, 2024
54dfbb3
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Sep 11, 2024
4098f94
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Sep 25, 2024
d83ad93
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Oct 15, 2024
2c57792
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Oct 18, 2024
e2b3b11
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Oct 31, 2024
edc0670
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Oct 31, 2024
590d616
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Nov 4, 2024
9a528a6
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Nov 5, 2024
203dbfa
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Nov 8, 2024
2e925a8
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Nov 13, 2024
d2134af
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Nov 13, 2024
85cda1d
Merge remote-tracking branch 'upstream/develop' into pr/test_fixes
ximinez Nov 27, 2024
4e38577
Merge branch 'develop' into pr/test_fixes
ximinez Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/test/jtx/impl/flags.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ void
nflags::operator()(Env& env) const
{
auto const sle = env.le(account_);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The data members flags::account_ and nflags::account_ always hold a valid instance of the Account class. These data members are not pointers or std::optional values.

Hence, env.le(account_) must always exist, isn't it? Are you envisioning a future modification to these flags and nflags classes, due to which a null pointer might be returned here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A valid instance of an Account does not guarantee that that account has been created / funded on the ledger. So even though the Account is valid, a null sle could easily be returned. Consider this test:

Env env{*this};
Account alice("alice");

env.require(flags(alice, asfRequireDest));
env.require(nflags(alice, asfRequireAuth));

That's a perfectly validly written test, but without this change, it'll dereference a null SLE and crash. With this change, the test will fail in the changed functions, but it won't crash.

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay, thanks for the clarification.

if (sle->isFieldPresent(sfFlags))
if (!sle)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Before line 44 (above), for the sake of consistency, we need to introduce a similar check

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Fixed.

env.test.fail();
else if (sle->isFieldPresent(sfFlags))
env.test.expect((sle->getFieldU32(sfFlags) & mask_) == 0);
else
env.test.pass();
Expand Down
2 changes: 1 addition & 1 deletion src/test/rpc/AccountSet_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class AccountSet_test : public beast::unit_test::suite
env.fund(XRP(10000), noripple(alice));
// ask for the ledger entry - account root, to check its flags
auto const jrr = env.le(alice);
BEAST_EXPECT((*env.le(alice))[sfFlags] == 0u);
BEAST_EXPECT(jrr && jrr->at(sfFlags) == 0u);
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this file, there are 22 other dereferences of the *env.le() function. Visually, I'm convinced that none of them are null-pointers, because all the Account objects have been funded with some XRP.

But is it necessary to perform this type of null-pointer-check on all of them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It wouldn't hurt to check. Even though it's been a while, I think it was the double call to env.le() that caught my attention, and then I just checked for null since I was changing it anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I realize that I did note "Maybe there are more of these?" in the "Future Tasks" section of the description.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok

}

void
Expand Down
Loading