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.

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

Update codebase from boost::string_view into std::string_view #4509

Merged
merged 33 commits into from
Jun 17, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
ba1da15
Migrated boost::string_view to std::string_view
ckeshava Apr 24, 2023
3850093
apply git-clang-format patch file from Github Actions
ckeshava Apr 24, 2023
1141e0f
use std::string_view.remove_suffix() member function to clear the val…
ckeshava Apr 24, 2023
c966149
[FOLD] address PR comments from Scott S and Nik
ckeshava Jun 23, 2023
efaf7ae
replace usages of boost::string_view with std::string_view. In turn, …
ckeshava Sep 11, 2023
92fda38
Merge branch 'develop' of https://github.com/XRPLF/rippled into updat…
ckeshava Sep 11, 2023
8c17a5c
clang format
ckeshava Sep 11, 2023
eada2c3
fix compiler errors due to the ServerHandlerImp -> ServerHandler change
ckeshava Sep 11, 2023
4967121
Merge branch 'develop' into updateStringView
ckeshava Jan 5, 2024
6dbfd7f
Merge branch 'develop' into updateStringView
ckeshava Jan 5, 2024
e4015fd
Update src/ripple/overlay/impl/Handshake.cpp
ckeshava Jan 5, 2024
be6e9be
removed old comment and an irrelevant file
ckeshava Jan 6, 2024
9d9f40e
partially addressed John's comments: Use std::string_view instead of …
ckeshava Jan 6, 2024
0c39f13
Merge branch 'updateStringView' of https://github.com/ckeshava/ripple…
ckeshava Jan 6, 2024
8da02e5
introduce template specialization for beast::LexicalCast<Out, std::st…
ckeshava Jan 6, 2024
a433207
refactor PeerImp::parseLedgerHash and base64_decode to use std::strin…
ckeshava Jan 6, 2024
2cf0eb8
refactor parseBase58 overloads to use std::string_view instead of std…
ckeshava Jan 6, 2024
85e001d
address John's comments
ckeshava Jan 8, 2024
88412ea
include BOOST_BEAST_USE_STD_STRING macro in the build system. Hence, …
ckeshava Jan 8, 2024
b780312
remove unused include statements
ckeshava Jan 8, 2024
999d9c2
build instructions to configure boost to use std::string_view
ckeshava Jan 8, 2024
cee5ff8
address John's PR comments: Update the Build instructions, include a …
ckeshava Jan 16, 2024
ac77412
Merge branch 'develop' into updateStringView
ckeshava Jan 16, 2024
cfa960f
replace the usage of std::string_view const& to std::string_view (pas…
ckeshava Jan 26, 2024
5313b13
Merge branch 'develop' into updateStringView
ckeshava Jan 26, 2024
204f9f4
construct a std:;string instead of using the assignment operator
ckeshava Feb 2, 2024
d851b16
Merge branch 'develop' into updateStringView
ckeshava Feb 2, 2024
49968bf
do not return a std::string_view from functions for safety reasons - …
ckeshava Feb 22, 2024
a20907f
Merge branch 'develop' into updateStringView
ckeshava Feb 22, 2024
d3d309a
Merge branch 'develop' into updateStringView
ckeshava Feb 28, 2024
fcf1375
Merge branch 'develop' into updateStringView
ckeshava Mar 6, 2024
3cda316
- rectify the mistakes in the merge from base58 changes.
ckeshava Mar 7, 2024
ba99ab4
Merge branch 'develop' into updateStringView
seelabs Jun 17, 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
2 changes: 1 addition & 1 deletion src/ripple/app/misc/ValidatorList.h
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,7 @@ class ValidatorList
*/
std::optional<Json::Value>
getAvailable(
boost::beast::string_view const& pubKey,
std::string_view pubKey,
std::optional<std::uint32_t> forceVersion = {});

/** Return the number of configured validator list sites. */
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/app/misc/impl/ValidatorList.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1676,7 +1676,7 @@ ValidatorList::for_each_available(

std::optional<Json::Value>
ValidatorList::getAvailable(
boost::beast::string_view const& pubKey,
std::string_view pubKey,
std::optional<std::uint32_t> forceVersion /* = {} */)
{
std::shared_lock read_lock{mutex_};
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/basics/StringUtilities.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ strUnHex(std::string const& strSrc)
}

inline std::optional<Blob>
strViewUnHex(boost::string_view const& strSrc)
strViewUnHex(std::string_view strSrc)
{
return strUnHex(strSrc.size(), strSrc.cbegin(), strSrc.cend());
}
Expand Down
8 changes: 6 additions & 2 deletions src/ripple/overlay/impl/OverlayImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -997,9 +997,13 @@ OverlayImpl::processValidatorList(
return true;
};

auto key = req.target().substr(prefix.size());
Copy link
Contributor

@nbougalis nbougalis Apr 25, 2023

Choose a reason for hiding this comment

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

I'd personally leave this alone. You're adding complexity for no real reason 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.

If I leave type-deduction to auto, I'm not 100% sure if it is deduced as std::string_view. On line 1006, there is a comparison of key with std::string_view::npos.

My observation is that construction of std::string_view without the second length parameter results in garbage values at the end of key. I'm not really sure why that's happening.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I sympathize with your position, @nbougalis. But key is passed to ValidatorList::getAvailable(), which accepts a std::string_view. The conversion must happen somewhere.

I think perhaps the conversion can be done a bit more cleanly like this:

    // Convert the boost::string_view, returned by
    // boost::http::request::target(), into a std::string_view.
    std::string_view key = [&req, &prefix]() {
        boost::string_view const key = req.target().substr(prefix.size());
        return std::string_view(key.data(), key.length());
    }();

Since the auto is gone we can always see what we're handling. And it limits the scope of the boost::string_view as much as possible. But that doesn't remove the fundamental objection.

FWIW, I have a theory regarding the garbage values at the end of key if the length parameter is omitted. Remember that a string_view is a pointer and a length. There is no guarantee of a null termination to the string of characters contained by a string_view. This is one of the things that can make string_view dangerous to use.

If you construct a string_view with only a char*, then the string_view constructor must determine the length of the string it represents. It does so by scanning for a null termination. Since the old string_view you are using to construct the new string_view is not required to contain a null terminated string, the constructor of the new string_view may walk past the end of valid characters, potentially resulting in undefined behavior.

For more details see the description of constructor 4 on this page: https://en.cppreference.com/w/cpp/string/basic_string_view/basic_string_view

The lesson from this is that the only situation where a string_view should be constructed with a char* (and no length) is if you know the char* is a c-style string with a null termination.

string_view is often a nice optimization. But it's easy to mess up with it. We must handle the knife carefully.

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 missed this comment, thanks for the explanation Scott! yes, that makes sense

// Explicitly construct a std::string_view until http_request_type is
// migrated from boost
std::string_view key{
req.target().substr(prefix.size()).data(),
req.target().substr(prefix.size()).length()};

if (auto slash = key.find('/'); slash != boost::string_view::npos)
if (auto slash = key.find('/'); slash != std::string_view::npos)
{
auto verString = key.substr(0, slash);
if (!boost::conversion::try_lexical_convert(verString, version))
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/resource/ResourceManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class Manager : public beast::PropertyStream::Source
newInboundEndpoint(
beast::IP::Endpoint const& address,
bool const proxy,
boost::string_view const& forwardedFor) = 0;
std::string_view forwardedFor) = 0;

/** Create a new endpoint keyed by outbound IP address and port. */
virtual Consumer
Expand Down
5 changes: 2 additions & 3 deletions src/ripple/resource/impl/ResourceManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,13 @@ class ManagerImp : public Manager
newInboundEndpoint(
beast::IP::Endpoint const& address,
bool const proxy,
boost::string_view const& forwardedFor) override
std::string_view forwardedFor) override
{
if (!proxy)
return newInboundEndpoint(address);

boost::system::error_code ec;
auto const proxiedIp =
boost::asio::ip::make_address(forwardedFor.to_string(), ec);
auto const proxiedIp = boost::asio::ip::make_address(forwardedFor, ec);
if (ec)
{
journal_.warn()
Expand Down
4 changes: 2 additions & 2 deletions src/ripple/rpc/Context.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ struct JsonContext : public Context
*/
struct Headers
{
boost::string_view user;
boost::string_view forwardedFor;
std::string_view user;
std::string_view forwardedFor;
};

Json::Value params;
Expand Down
8 changes: 4 additions & 4 deletions src/ripple/rpc/Role.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,15 @@ requestRole(
Port const& port,
Json::Value const& params,
beast::IP::Endpoint const& remoteIp,
boost::string_view const& user);
std::string_view user);

Resource::Consumer
requestInboundEndpoint(
Resource::Manager& manager,
beast::IP::Endpoint const& remoteAddress,
Role const& role,
boost::string_view const& user,
boost::string_view const& forwardedFor);
std::string_view user,
std::string_view forwardedFor);

/**
* Check if the role entitles the user to unlimited resources.
Expand All @@ -85,7 +85,7 @@ ipAllowed(
std::vector<boost::asio::ip::network_v4> const& nets4,
std::vector<boost::asio::ip::network_v6> const& nets6);

boost::string_view
std::string_view
forwardedFor(http_request_type const& request);

} // namespace ripple
Expand Down
7 changes: 4 additions & 3 deletions src/ripple/rpc/handlers/Ping.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,14 @@ doPing(RPC::JsonContext& context)
break;
case Role::IDENTIFIED:
ret[jss::role] = "identified";
ret[jss::username] = context.headers.user.to_string();
// Can I avoid making expensive copies for std::string?
ret[jss::username] = std::string{context.headers.user};
if (context.headers.forwardedFor.size())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@scottschurr I couldn't think of a cheaper solution for this piece of code.
std::string{context.headers.user.data()} seems to include garbage trailing data

Copy link
Collaborator

Choose a reason for hiding this comment

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

At this level of the source code I'm not seeing boost::string_view used here. Naively, it seems like you can just leave this alone? Am I missing something?

Remember, there are lots of places that std::string_view is not safe to use. We should not try to replace uses of std::string with std::string_view unless we're really sure it is safe. I suggest you not try any replacements of std::string with std::string_view in this initial pass.

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 understand. context.headers.user and context.headers.forwardedFor are of type boost::string_view in the current version. Source: src/ripple/rpc/Context.h:60

I'm trying to port from this dependence on boost to std.

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 clarification. I don't think there's currently any way to make this cheaper than what you have done here. The Json::Value::operator[] interface does not (yet) allow assignment from a std::string_view.But it could be made string_view aware in a future pull request if we chose to do that.

Alternatively, we could update the rippled codebase to use boost::json instead of Json::Value. I think that would also get us std::string_view support. But that would be a much bigger lift (with potentially more benefit).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, thanks for the suggestions Scott!
Yeah, Json::Value was written a long time, I'm guessing before boost came up with their json library. It appears to have so much of overlap with implementations from boost!

ret[jss::ip] = context.headers.forwardedFor.to_string();
ret[jss::ip] = std::string{context.headers.forwardedFor};
break;
case Role::PROXY:
ret[jss::role] = "proxied";
ret[jss::ip] = context.headers.forwardedFor.to_string();
ret[jss::ip] = std::string{context.headers.forwardedFor};
default:;
}

Expand Down
36 changes: 19 additions & 17 deletions src/ripple/rpc/impl/Role.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ requestRole(
Port const& port,
Json::Value const& params,
beast::IP::Endpoint const& remoteIp,
boost::string_view const& user)
std::string_view user)
{
if (isAdmin(port, params, remoteIp.address()))
return Role::ADMIN;
Expand Down Expand Up @@ -142,8 +142,8 @@ requestInboundEndpoint(
Resource::Manager& manager,
beast::IP::Endpoint const& remoteAddress,
Role const& role,
boost::string_view const& user,
boost::string_view const& forwardedFor)
std::string_view user,
std::string_view forwardedFor)
{
if (isUnlimited(role))
return manager.newUnlimitedEndpoint(remoteAddress);
Expand All @@ -152,18 +152,18 @@ requestInboundEndpoint(
remoteAddress, role == Role::PROXY, forwardedFor);
}

static boost::string_view
extractIpAddrFromField(boost::string_view field)
static std::string_view
extractIpAddrFromField(std::string_view field)
{
// Lambda to trim leading and trailing spaces on the field.
auto trim = [](boost::string_view str) -> boost::string_view {
boost::string_view ret = str;
auto trim = [](std::string_view str) -> std::string_view {
std::string_view ret = str;

// Only do the work if there's at least one leading space.
if (!ret.empty() && ret.front() == ' ')
{
std::size_t const firstNonSpace = ret.find_first_not_of(' ');
if (firstNonSpace == boost::string_view::npos)
if (firstNonSpace == std::string_view::npos)
// We know there's at least one leading space. So if we got
// npos, then it must be all spaces. Return empty string_view.
return {};
Expand All @@ -178,7 +178,7 @@ extractIpAddrFromField(boost::string_view field)
c == ' ' || c == '\r' || c == '\n')
{
std::size_t const lastNonSpace = ret.find_last_not_of(" \r\n");
if (lastNonSpace == boost::string_view::npos)
if (lastNonSpace == std::string_view::npos)
// We know there's at least one leading space. So if we
// got npos, then it must be all spaces.
return {};
Expand All @@ -189,7 +189,7 @@ extractIpAddrFromField(boost::string_view field)
return ret;
};

boost::string_view ret = trim(field);
std::string_view ret = trim(field);
if (ret.empty())
return {};

Expand Down Expand Up @@ -251,13 +251,13 @@ extractIpAddrFromField(boost::string_view field)

// If there's a port appended to the IP address, strip that by
// terminating at the colon.
if (std::size_t colon = ret.find(':'); colon != boost::string_view::npos)
if (std::size_t colon = ret.find(':'); colon != std::string_view::npos)
ret = ret.substr(0, colon);

return ret;
}

boost::string_view
std::string_view
forwardedFor(http_request_type const& request)
{
// Look for the Forwarded field in the request.
Expand Down Expand Up @@ -286,10 +286,9 @@ forwardedFor(http_request_type const& request)

// We found a "for=". Scan for the end of the IP address.
std::size_t const pos = [&found, &it]() {
std::size_t pos =
boost::string_view(found, it->value().end() - found)
.find_first_of(",;");
if (pos != boost::string_view::npos)
std::size_t pos = std::string_view(found, it->value().end() - found)
.find_first_of(",;");
if (pos != std::string_view::npos)
return pos;

return it->value().size() - forStr.size();
Expand All @@ -305,7 +304,10 @@ forwardedFor(http_request_type const& request)
std::size_t found = it->value().find(',');
if (found == boost::string_view::npos)
found = it->value().length();
return extractIpAddrFromField(it->value().substr(0, found));
// http_request_type has a dependency on boost. Explicitly convert such
// boost::string_view into std::string_view
return extractIpAddrFromField(
std::string_view{it->value().substr(0, found).data(), found});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you're doing a bit more work than necessary here. Since the substryou are building has a starting position of 0, we know that value and substr will return the same value for data(). So I think you can remove the call to substr() like this:

        // http_request_type has a dependency on boost. Explicitly convert such
        // boost::string_view into std::string_view
        return extractIpAddrFromField(
            std::string_view{it->value().data(), found});

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, this makes sense 👍

}

return {};
Expand Down
19 changes: 13 additions & 6 deletions src/ripple/rpc/impl/ServerHandlerImp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -559,8 +559,9 @@ ServerHandlerImp::processSession(
[&] {
auto const iter = session->request().find("X-User");
if (iter != session->request().end())
return iter->value();
return boost::beast::string_view{};
return std::string_view{
iter->value().data(), iter->value().length()};
return std::string_view{};
}());
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want you can remove the necessity of typing std::string_view twice by specifying the return type of the lambda. The resulting lambda might look like this:

        [&session]() -> std::string_view {
            auto const iter = session->request().find("X-User");
            if (iter != session->request().end())
                return {iter->value().data(), iter->value().length()};
            return {};
        }());

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thank you very much Scott! this looks nicer 👍


if (beast::rfc2616::is_keep_alive(session->request()))
Expand Down Expand Up @@ -592,8 +593,8 @@ ServerHandlerImp::processRequest(
beast::IP::Endpoint const& remoteIPAddress,
Output&& output,
std::shared_ptr<JobQueue::Coro> coro,
boost::string_view forwardedFor,
boost::string_view user)
std::string_view forwardedFor,
std::string_view user)
{
auto rpcJ = app_.journal("RPC");

Expand Down Expand Up @@ -847,8 +848,14 @@ ServerHandlerImp::processRequest(
*/
if (role != Role::IDENTIFIED && role != Role::PROXY)
{
forwardedFor.clear();
user.clear();
// Simulate the clearing of string_view through a swap. Is this a
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'm not comfortable with this method. Since these strings (user and forwardedFor) are being modified, they are not suitable candidates for std::string_view. boost::string_view has a clear() member function, until std. Should I retain the original version or change this occurence to std:string instead? @scottschurr

Copy link
Collaborator

@scottschurr scottschurr Apr 24, 2023

Choose a reason for hiding this comment

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

std::string_view doesn't have a clear() method, but it does have a remove_suffix() method: https://en.cppreference.com/w/cpp/string/basic_string_view/remove_suffix

I think you might be able to use use...

forwardedFor.remove_suffix(forwardedFor.size())

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes thanks, this looks much cleaner 👍 I'll incorporate this change.

// good candidate for std::string_view? This is due to a discrepency
// between boost::string_view versus std::string_view's APIs
// forwardedFor.clear();
std::string_view empty_str;
forwardedFor.swap(empty_str);
// user.clear();
user.swap(empty_str);
}

JLOG(m_journal.debug()) << "Query: " << strMethod << params;
Expand Down
4 changes: 2 additions & 2 deletions src/ripple/rpc/impl/ServerHandlerImp.h
thejohnfreeman marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,8 @@ class ServerHandlerImp
beast::IP::Endpoint const& remoteIPAddress,
Output&&,
std::shared_ptr<JobQueue::Coro> coro,
boost::string_view forwardedFor,
boost::string_view user);
std::string_view forwardedFor,
std::string_view user);

Handoff
statusResponse(http_request_type const& request) const;
Expand Down
4 changes: 2 additions & 2 deletions src/ripple/rpc/impl/WSInfoSub.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,13 @@ class WSInfoSub : public InfoSub
}
}

boost::string_view
std::string_view
user() const
{
return user_;
}

boost::string_view
std::string_view
forwarded_for() const
{
return fwdfor_;
Expand Down