test: Check for some unlikely null dereferences in tests

[ximinez]

High Level Overview of Change

I stumbled upon these couple of places where test code could, but is very unlikely to, dereference a null pointer. I added a simple check to each of them.

Context of Change

This was a random find.

Even though these changes are tiny, I don't think they are insignificant enough to be considered "trivial", so I'm requesting two reviews.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• Tests (you added tests for code that already exists, or your new feature included in this PR)

Before / After

There should be no observable change, as tests aren't currently likely / able to get a null value.

Future Tasks

Maybe there are more of these?

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.9%. Comparing base (b54d85d) to head (4e38577).
Report is 4 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5004     +/-   ##
=========================================
- Coverage     77.9%   77.9%   -0.0%     
=========================================
  Files          784     784             
  Lines        66681   66682      +1     
  Branches      8140    8137      -3     
=========================================
- Hits         51948   51944      -4     
- Misses       14733   14738      +5     

see 6 files with indirect coverage changes

[ckeshava]

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

[ximinez]

Good catch. Fixed.

[ckeshava]

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?

[ximinez]

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.

[ckeshava]

okay, thanks for the clarification.

[vlntb]

LGTM

[ckeshava]

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?

[ximinez]

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.

[ximinez]

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

[ckeshava]

ok

[ckeshava]

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()

[ximinez]

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().

[ckeshava]

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)

[ximinez]

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.

[ckeshava]

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.

[ximinez]

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.

[ckeshava]

okay 👍 makes sense

[ximinez]

Hold pending 2.3.0 release.

[ximinez]

Manually merged as 8215c60

$ git diff --exit-code 8215c605b456d16ce041f5fc579e0dfdaf415576 ximinez/pr/test_fixes && echo Identical
Identical