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

Improve STVector256 deserialization #4204

Closed
wants to merge 2 commits into from
Closed

Improve STVector256 deserialization #4204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c497a62
Improve STVector256 deserialization
nbougalis Jun 12, 2022
ba5503f
[FOLD] Address review comments
nbougalis Jun 30, 2022
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
7 changes: 4 additions & 3 deletions src/ripple/basics/Slice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ class Slice
std::size_t size_ = 0;

public:
using const_iterator = std::uint8_t const*;
using value_type = std::uint8_t;
using const_iterator = value_type const*;

/** Default constructed Slice has length 0. */
Slice() noexcept = default;
Expand All @@ -75,13 +76,13 @@ class Slice
This may be zero for an empty range.
*/
/** @{ */
std::size_t
[[nodiscard]] std::size_t
size() const noexcept
{
return size_;
}

std::size_t
[[nodiscard]] std::size_t
length() const noexcept
{
return size_;
Expand Down
6 changes: 6 additions & 0 deletions src/ripple/basics/base_uint.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#define RIPPLE_BASICS_BASE_UINT_H_INCLUDED

#include <ripple/basics/Expected.h>
#include <ripple/basics/Slice.h>
#include <ripple/basics/contract.h>
#include <ripple/basics/hardened_hash.h>
#include <ripple/basics/strHex.h>
Expand Down Expand Up @@ -56,6 +57,11 @@ struct is_contiguous_container<
{
};

template <>
struct is_contiguous_container<Slice> : std::true_type
{
};

} // namespace detail

/** Integers of any length that is a multiple of 32-bits
Expand Down
26 changes: 13 additions & 13 deletions src/ripple/protocol/impl/STVector256.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,19 @@ namespace ripple {

STVector256::STVector256(SerialIter& sit, SField const& name) : STBase(name)
{
Blob data = sit.getVL();
auto const count = data.size() / (256 / 8);
mValue.reserve(count);
Blob::iterator begin = data.begin();
unsigned int uStart = 0;
for (unsigned int i = 0; i != count; i++)
{
unsigned int uEnd = uStart + (256 / 8);
// This next line could be optimized to construct a default
// uint256 in the vector and then copy into it
mValue.push_back(uint256(Blob(begin + uStart, begin + uEnd)));
uStart = uEnd;
}
auto const slice = sit.getSlice(sit.getVLDataLength());

if (slice.size() % uint256::size() != 0)
Throw<std::runtime_error>(
"Bad serialization for STVector256: " +
std::to_string(slice.size()));
Copy link
Collaborator

@seelabs seelabs Jun 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The NFTokenCancelOffer uses a STVector256 field. The old code may have accepted an incorrectly serialized sfNFTTokenOffers field where as the new code throws - which would make this transaction breaking. I'd be surprised if an incorrectly serialized transaction could ever make into the ledger, but I'm not sure I can prove it (code like this would assert in debug mode: https://github.com/nbougalis/rippled/blob/c497a62fb95c98a1e29f0eb0557ca36adcb89c7d/src/ripple/app/tx/impl/Transactor.cpp#L785-L802)

I think it's fine as-is, but I did want to bring this up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for bringing this up. So two comments:

  1. This cannot occur on the main network since the featureNonFungibleTokensV1 amendment isn't active, and so the NFTokenCancelOffer transaction cannot be executed.
  2. In addition to the code you showed, the code will call sterilize on submitted transactions, which should detect this in some circumstances.

Given than we're planning to introduce another fix amendment to correct a minor issue identified with the XLS-20 implementation, which (for obvious reasons!) should be activated before XLS-20 implementation, I think my suggestion would be to include this code with this release. It doesn't need to be directly gated by the amendment, but if the fix amendment activates before featureNonFungibleTokensV1 (and again, it should), then this issue will never be able to trigger.

Apropos of this, I wonder what the performance implications of always executing the serialization/deserialization check that's currently only done if compiled with -DDEBUG would be. Also, I wonder whether this check more properly belongs in either preflight or preclaim (we ought to claim a fee if someone submits a broken transaction and entomb the transation in the ledger!).

There are good reasons to have it in operator() (i.e. during "application") too, but, as I said above, if this check fails, it would be better to tec out of there, instead of simply calling assert.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the clarifications. Yeah, definitely fine as-is. 👍


auto const cnt = slice.size() / uint256::size();

mValue.reserve(cnt);

for (std::size_t i = 0; i != cnt; ++i)
mValue.emplace_back(slice.substr(i * uint256::size(), uint256::size()));
}

STBase*
Expand Down