basic_parser_impl: make sentinel() return a unique pointer #814

MaxKellermann · 2022-11-21T14:01:19Z

Right now, sentinel() casts the basic_parser_impl pointer (this) to const char *, but that pointer is not unique if the input buffer happens to be placed right before the basic_parser_impl instance - the end of that buffer then has the same address as basic_parser_impl.

Example code:

  struct {
    char buffer[8]{"{\"12345\""};
    boost::json::stream_parser p;
  } s;
  s.p.write(s.buffer, sizeof(s.buffer));
  s.p.write(":0}", 3);

This stops parsing at the end of the buffer, and then the incomplete() check in parse_string() will return true; the second write() call will crash with assertion failure:

boost/json/basic_parser_impl.hpp:1016: const char* boost::json::basic_parser::parse_unescaped(const char*, std::integral_constant<bool, StackEmpty_>, std::integral_constant<bool, AllowComments_>, bool) [with bool StackEmpty_ = true; bool IsKey_ = true; Handler = boost::json::detail::handler]: Assertion `*cs == '\x22'' failed.

This changes sentinel() to return the address of a static variable instead, which is guaranteed to be unique.

codecov · 2022-11-21T14:32:09Z

Codecov Report

Merging #814 (0f987f0) into develop (6af3a5e) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #814   +/-   ##
========================================
  Coverage    99.00%   99.00%           
========================================
  Files           71       71           
  Lines         6832     6832           
========================================
  Hits          6764     6764           
  Misses          68       68

Impacted Files	Coverage Δ
include/boost/json/basic_parser_impl.hpp	`98.33% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6af3a5e...0f987f0. Read the comment docs.

vinniefalco · 2022-11-21T14:32:12Z

Very interesting change :) @sdkrystian what do you think about this?

grisumbras · 2022-11-21T15:26:32Z

This should work, but may affect performance. I wanted to see automatic benchmark results. @MaxKellermann also please add a test that's failing without your change (use one commit).

grisumbras · 2022-11-21T15:29:23Z

Btw, alternative implementation would be to return the address of any parser's member except the first.

MaxKellermann · 2022-11-21T15:42:16Z

Btw, alternative implementation would be to return the address of any parser's member except the first.

That's fragile because it can break when you reorder the members. But you could add +1 to the char-casted this pointer.

MaxKellermann · 2022-11-21T15:59:19Z

I added a failing unit test and I changed the implementation to return (char*)this + 1; this was necessary because my first approach was still faulty - theoretically (though unlikely), this global variable could be the end of a global buffer, leading to the same crash as before.

grisumbras · 2022-11-21T16:28:01Z

this + 1 will end up within the handler. So, I wonder if someone may try feeding the parser with something within the handler.

MaxKellermann · 2022-11-21T16:33:18Z

this + 1 will end up within the handler

True; but note that without this PR, it was already within the handler, so this PR is an improvement for sure.

What about using nullptr as sentinel? That would be cheaper than comparing pointers with this (i.e. with a CPU register). Are there corner cases where a valid non-sentinel value can be nullptr? (This idea is so obvious that I thought there must be a reason why this wasn't already done.)

cppalliance-bot · 2022-11-21T17:09:44Z

Benchmark test results. More info at https://benchmark.cppalliance.org/jsonbenchmarks-pullrequests/814/pullrequest.html

grisumbras · 2022-11-22T05:59:38Z

Can you please squash the commits?

MaxKellermann · 2022-11-22T08:53:51Z

Can you please squash the commits?

Done.

cppalliance-bot · 2022-11-22T10:09:38Z

Benchmark test results. More info at https://benchmark.cppalliance.org/jsonbenchmarks-pullrequests/814/pullrequest.html

sdkrystian · 2022-11-22T21:04:10Z

@vinniefalco this reason why sentinel returned this was because it would be already loaded in some register when the function was called, so it could simply be moved into rax via a register-to-register move (which don''t require any execution resources on modern CPUs). So while this fix may work, it may just be better to return nullptr (which I believe should work; possibly even better than the old implementation).

vinniefalco · 2022-11-22T21:11:00Z

Interesting!!

MaxKellermann · 2022-11-22T21:14:04Z

it may just be better to return nullptr (which I believe should work; possibly even better than the old implementation).

I had the same idea yesterday, but

This idea is so obvious that I thought there must be a reason why this wasn't already done

If you confirm there isn't any reason, I'll happily change this PR to nullptr!

vinniefalco · 2022-11-23T15:46:18Z

try nullptr

MaxKellermann · 2022-11-23T15:52:54Z

try nullptr

That fails lots of unit tests:

====== BEGIN OUTPUT ======
boost.json.parse
#3 parse.cpp(49) failed: ! ec
#6 parse.cpp(49) failed: ! ec
#9 parse.cpp(49) failed: ! ec
#12 parse.cpp(49) failed: ! ec
#15 parse.cpp(49) failed: ! ec
#18 parse.cpp(49) failed: ! ec
terminate called after throwing an instance of 'boost::wrapexcept<boost::system::system_error>'
  what():  incomplete JSON [boost.json:3 at ../../boost/json/basic_parser_impl.hpp:2753 in function 'write_some']
Aborted (core dumped)

EXIT STATUS: 134
====== END OUTPUT ======
====== BEGIN OUTPUT ======
boost.json.value
#1023 value.cpp(2283) failed: jv == 23
#1024 value.cpp(2288) failed: jv == 23
#1026 value.cpp(2294) failed: jv == 23
#1028 value.cpp(2300) failed: jv == 23
#1030 value.cpp(2307) failed: jv == 23
#1033 value.cpp(2315) failed: jv == 23
3ms, 1 suites, 6 failures, 1034 total.

EXIT STATUS: 1
====== END OUTPUT ======

... and many more. I did not investigate further.

vinniefalco · 2022-11-23T16:17:01Z

Another possibility is to privately derive basic_parser_impl from a struct that has a single private char in it:

class detail::pad
{
  char c__ = 0;
};

MaxKellermann · 2022-11-23T16:18:33Z

Another possibility is to privately derive basic_parser_impl from a struct that has a single private char in it:

You'd need to have at least two chars, and return a pointer to the second one, to ensure that no preceding buffer can end at the returned pointer, which is the whole problem here.

vinniefalco · 2022-11-23T17:10:02Z

You'd need to have at least two chars, and return a pointer to the second one, to ensure that no preceding buffer can end at the returned pointer, which is the whole problem here.

Well no, because sentinel() is a member of basic_parser_impl so this would refer to the basic_parser_impl part and not the detail::pad part.

struct pad
{
  char c;
};

struct parser : private pad
{
  char const* sentinel() const noexcept
  {
    return this; // one past `pad`
  }
};

MaxKellermann · 2022-11-23T18:13:32Z

Well no, because sentinel() is a member of basic_parser_impl so this would refer to the basic_parser_impl part and not the detail::pad part.

They have the same address in your example (most likely, but that may depend on the compiler-specific C++ ABI).

one past pad

No. Let me godbolt this for you :-) https://godbolt.org/z/63T8c4vx8

vinniefalco · 2022-11-23T23:38:34Z

:( That is rather unfortunate

MaxKellermann · 2022-11-24T09:27:37Z

If you want it watertight, I can change this PR to add a char sentinel[2] field and return &sentinel[1]. Do you want that?
For my taste, the current state or the PR is "good enough", but it's not (strictly) watertight.
I'd love to have nullptr, but that may require fixing some existing quirks - if you want that, I can give that a try.
Let me know your preference.

grisumbras · 2022-11-24T16:54:05Z

How would that be better than just returning an address to any member? Or this + 1?

MaxKellermann · 2022-11-24T17:02:50Z

How would that be better than just returning an address to any member?

If you return the address of any member, it's only watertight if you can guarantee that this member is not directly preceded by a buffer to be parsed. You can look at the current layout of the class and speculate it's not, but that's fragile; if eventually the code gets refactored and members get reordered (which is a perfectly reasonable thing to do), this assumption may no longer be true and the code may break.

Having a char sentinel[2] and using &sentinel[1] guarantees that the pointer is never preceded by a buffer, because &sentinel[1] is guaranteed to be preceded by &sentinel[0], no matter which C++ ABI, and no matter how you reorder fields. This is the strictly watertight solution.

Or this + 1?

It was you who wrote: "this + 1 will end up within the handler. So, I wonder if someone may try feeding the parser with something within the handler."
It's not whatertight. It's only safe against parsing buffers outside of the parser object, but not against buffers which happen to be inside.

vinniefalco · 2022-11-24T18:45:42Z

( reinterpret_cast<char const*>(this) + 1 )

WPMGPRoSToTeMa · 2022-11-24T19:01:25Z

include/boost/json/basic_parser_impl.hpp

@@ -217,8 +217,10 @@ const char*
 basic_parser<Handler>::
 sentinel()
 {
+    // the "+1" ensures that the returned pointer is unique even if
+    // the given input buffer borders on this object
    return reinterpret_cast<


static_assert(sizeof(*this) > 1);

class basic_parser has lots of fields, and is always larger than 1 byte, unless this class gets refactored and all fields get removed, which isn't going to happen. I don't think this static_assert adds real value, it's trivial to see it's always true.

Agreed, lol

grisumbras · 2022-11-28T12:25:48Z

The worst part of making sentinel() return nullptr is that it will put a precondition on write_some (you wouldn't be able to do parser.write_some(true, nullptr, 0, ec). This isn't necessarily bad, but when GCC started optimising around a similar precondition in memcpy, it broke a lot of code.

I would stick to returning a pointer into parser, even if that means some reduction in performance. I also don't like adding unnecessary fields just for this purpose, I would prefer returning a pointer to an existing field. We already keep layout of value et al. fixed, doing something similar for basic_parser shouldn't be a problem.

On the other hand, I'm not necessarily against returning this + 1, but we might want to document the fact that people shouldn't feed to the parser buffers inside handler.

@vinniefalco your thoughts?

grisumbras · 2022-12-30T12:30:55Z

@MaxKellermann can you please rebase on current develop and re-push. I will merge it then.

Right now, sentinel() casts the `basic_parser` pointer (`this`) to `const char *`, but that pointer is not unique if the input buffer happens to be placed right before the `basic_parser_impl` instance - the end of that buffer then has the same address as `basic_parser`. Example code: ``` struct { char buffer[8]{"{\"12345\""}; boost::json::stream_parser p; } s; s.p.write(s.buffer, sizeof(s.buffer)); s.p.write(":0}", 3); ``` This stops parsing at the end of the buffer, and then the `incomplete()` check in `parse_string()` will return true; the second `write()` call will crash with assertion failure: > boost/json/basic_parser_impl.hpp:1016: const char* boost::json::basic_parser<Handler>::parse_unescaped(const char*, std::integral_constant<bool, StackEmpty_>, std::integral_constant<bool, AllowComments_>, bool) [with bool StackEmpty_ = true; bool IsKey_ = true; Handler = boost::json::detail::handler]: Assertion `*cs == '\x22'' failed. This changes `sentinel()` by adding 1 to guaranteed that the sentinel pointer is unique even if the input buffers borders on this object.

MaxKellermann · 2022-12-30T14:19:30Z

@MaxKellermann can you please rebase on current develop and re-push. I will merge it then.

done

cppalliance-bot · 2022-12-30T15:34:38Z

Benchmark test results. More info at https://benchmark.cppalliance.org/jsonbenchmarks-pullrequests/814/pullrequest.html

grisumbras · 2022-12-30T19:45:30Z

Thank you for your contribution.

MaxKellermann force-pushed the unique_sentinel branch from 9562730 to 91de423 Compare November 21, 2022 14:11

MaxKellermann force-pushed the unique_sentinel branch from 91de423 to 7e2dfa8 Compare November 21, 2022 15:57

MaxKellermann force-pushed the unique_sentinel branch from 7e2dfa8 to 062b93d Compare November 22, 2022 08:53

WPMGPRoSToTeMa reviewed Nov 24, 2022

View reviewed changes

grisumbras mentioned this pull request Dec 30, 2022

Assertion failure in basic_parser under mingw-gcc and clang-win 32 bit #828

Closed

MaxKellermann force-pushed the unique_sentinel branch from 062b93d to 0f987f0 Compare December 30, 2022 14:18

grisumbras merged commit 0f987f0 into boostorg:develop Dec 30, 2022

MaxKellermann deleted the unique_sentinel branch October 5, 2023 08:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

basic_parser_impl: make sentinel() return a unique pointer #814

basic_parser_impl: make sentinel() return a unique pointer #814

MaxKellermann commented Nov 21, 2022 •

edited

codecov bot commented Nov 21, 2022 •

edited

vinniefalco commented Nov 21, 2022 •

edited

grisumbras commented Nov 21, 2022

grisumbras commented Nov 21, 2022

MaxKellermann commented Nov 21, 2022

MaxKellermann commented Nov 21, 2022

grisumbras commented Nov 21, 2022

MaxKellermann commented Nov 21, 2022 •

edited

cppalliance-bot commented Nov 21, 2022

grisumbras commented Nov 22, 2022

MaxKellermann commented Nov 22, 2022

cppalliance-bot commented Nov 22, 2022

sdkrystian commented Nov 22, 2022

vinniefalco commented Nov 22, 2022

MaxKellermann commented Nov 22, 2022

vinniefalco commented Nov 23, 2022

MaxKellermann commented Nov 23, 2022

vinniefalco commented Nov 23, 2022

MaxKellermann commented Nov 23, 2022

vinniefalco commented Nov 23, 2022

MaxKellermann commented Nov 23, 2022

vinniefalco commented Nov 23, 2022

MaxKellermann commented Nov 24, 2022

grisumbras commented Nov 24, 2022

MaxKellermann commented Nov 24, 2022

vinniefalco commented Nov 24, 2022 •

edited

WPMGPRoSToTeMa Nov 24, 2022

MaxKellermann Nov 25, 2022

vinniefalco Nov 25, 2022

grisumbras commented Nov 28, 2022

grisumbras commented Dec 30, 2022

MaxKellermann commented Dec 30, 2022

cppalliance-bot commented Dec 30, 2022

grisumbras commented Dec 30, 2022

basic_parser_impl: make sentinel() return a unique pointer #814

basic_parser_impl: make sentinel() return a unique pointer #814

Conversation

MaxKellermann commented Nov 21, 2022 • edited

codecov bot commented Nov 21, 2022 • edited

Codecov Report

vinniefalco commented Nov 21, 2022 • edited

grisumbras commented Nov 21, 2022

grisumbras commented Nov 21, 2022

MaxKellermann commented Nov 21, 2022

MaxKellermann commented Nov 21, 2022

grisumbras commented Nov 21, 2022

MaxKellermann commented Nov 21, 2022 • edited

cppalliance-bot commented Nov 21, 2022

grisumbras commented Nov 22, 2022

MaxKellermann commented Nov 22, 2022

cppalliance-bot commented Nov 22, 2022

sdkrystian commented Nov 22, 2022

vinniefalco commented Nov 22, 2022

MaxKellermann commented Nov 22, 2022

vinniefalco commented Nov 23, 2022

MaxKellermann commented Nov 23, 2022

vinniefalco commented Nov 23, 2022

MaxKellermann commented Nov 23, 2022

vinniefalco commented Nov 23, 2022

MaxKellermann commented Nov 23, 2022

vinniefalco commented Nov 23, 2022

MaxKellermann commented Nov 24, 2022

grisumbras commented Nov 24, 2022

MaxKellermann commented Nov 24, 2022

vinniefalco commented Nov 24, 2022 • edited

WPMGPRoSToTeMa Nov 24, 2022

Choose a reason for hiding this comment

MaxKellermann Nov 25, 2022

Choose a reason for hiding this comment

vinniefalco Nov 25, 2022

Choose a reason for hiding this comment

grisumbras commented Nov 28, 2022

grisumbras commented Dec 30, 2022

MaxKellermann commented Dec 30, 2022

cppalliance-bot commented Dec 30, 2022

grisumbras commented Dec 30, 2022

MaxKellermann commented Nov 21, 2022 •

edited

codecov bot commented Nov 21, 2022 •

edited

vinniefalco commented Nov 21, 2022 •

edited

MaxKellermann commented Nov 21, 2022 •

edited

vinniefalco commented Nov 24, 2022 •

edited