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 all commits
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
8 changes: 6 additions & 2 deletions src/test/jtx/impl/flags.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ void
flags::operator()(Env& env) const
{
auto const sle = env.le(account_);
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.

Can we add a check inside the flags(Account const& account, Args... args) constructor, to check for null-pointer in account_ data member? It is not useful to set flags on a non-existent, unfunded account.

However, I've only observed the flags object passed as an input into env.require(). If that continues, there's probably no operational difference between the flags::constructor and the flags::operator()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. In the flags ctor, account_ is a value, not a pointer. It would be premature to check the ledger via an Env parameter because the ledger state may change in between the ctor and the call to operator().

Copy link
Collaborator

Choose a reason for hiding this comment

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

That may be true, but why would you ever want to construct a flags object with an unfunded account?
We can move the validity check up from the operator() into the constructor.

I can't think of situations where you'd need to create a flags(unfunded_account)

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's not that you would want to create a flags(unfunded_account), it's that the way these helper classes are constructed, there's no way to check whether the account is funded from the constructor.

Here's the definition of the ctor:

    template <class... Args>
    flags(Account const& account, Args... args)
        : flags_helper(args...), account_(account)
    {
    }

And here's the definition of the operator():

void
flags::operator()(Env& env) const
{
    auto const sle = env.le(account_);
    if (!sle)
        env.test.fail();
    else if (sle->isFieldPresent(sfFlags))
        env.test.expect((sle->getFieldU32(sfFlags) & mask_) == mask_);
    else
        env.test.expect(mask_ == 0);
}

The difference is the Env parameter to operator(). There isn't one of those available to the constructor, and we don't want one, because it would defeat the whole purpose of these helper classes and the env.require helper wrapper.

In other words, to do the check in the ctor, the callers would have to look something like

env.require(flags(env, alice, flag));

Which is ugly and redundant.

The last problem, IMHO, with trying to do a check like this in the ctor is what do you do if it fails? You could do some variation of a BEAST_EXPECT that would cause the test suite to fail, but then what? You'd either have an object with a dud account inside it that's going to get operator() called on it anyway, or you're going to have to throw an exception, which is what I was trying to avoid with this check in the first place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay, this is a helpful explanation.

Why do you want to avoid throwing an exception in the constructor? I vaguely remember reading about it, I think it has something to do with the cleanup of the dead object. If you know any resources/blogs, I'd like to read it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why do you want to avoid throwing an exception in the constructor?

There is nothing intrinsically wrong with throwing an exception in the ctor. It's a good way to handle certain types of errors where you want to prevent an object from being instantiated at all.

The reason I don't want to throw an exception in this ctor is because exceptions need to be caught or they terminate the whole program. The good news is that in the test suite, the tests are all wrapped so that exceptions will be caught, and treated as a test suite error. The bad news is that code is a blunt instrument, which doesn't tell you much about where and why the exception occurred. It also causes the rest of that suite to be skipped, as opposed to a failing BEAST_EXPECT, which logs a failure including file and line number, and finishes the rest of the suite. That makes debugging a lot easier.

The alternative is to wrap every single instantiation of flags in a try/catch block, which just adds unnecessary complication and syntactic garbage that doesn't add anything to the test.

So the short answer is that I don't want to throw in the ctor because it adds unnecessary complication when the check in operator() is perfectly sufficient.

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay 👍 makes sense

env.test.fail();
else if (sle->isFieldPresent(sfFlags))
env.test.expect((sle->getFieldU32(sfFlags) & mask_) == mask_);
else
env.test.expect(mask_ == 0);
Expand All @@ -51,7 +53,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