[boost] [Beast] Review

[Artyom Beilis via Boost]

Hello,

This is my formal review of Boost Beast.

> What is your evaluation of the design?

Also the general design is solid provides generic API that does the
job it defined.

The most problematic issue with Beast is it total reliance on static
polymorphism. This comes to sometimes very painful issues like:

Having the object code duplicated for different variants HTTP/HTTPS code
Having all the implementation in headers increasing compilation time
Adding complexity to end user since he needs to propagate the
templates to various places.

Virtually all concepts could be done with classic interfaces and
virtual functions.

Using of static polymorphism does not provide there performance
advantage but reduces code maintainability and adds lots of headache
to end user.

Note: it does not contradict of using ASIO but lots of stuff could be
pimpled or hidden having only few forward declarations of the relevant
classes.

> What is your evaluation of the implementation?

From what I seen didn't find may flows besides thous mentioned before
(content-length - that was fixed) and non-blocking sockets aren't
covered by tests.

However I didn't look too much since the code is quite hard to read
being too template heavy.

Second major issue is that it puts all the heavy work on end user
providing tiny parsing messaging layer that to be honest is the simple
part of HTTP handing.

It significantly limits its usability to virtually void.

> What is your evaluation of the documentation?

Documentation is OK - similar to ASIO - I think more examples should
be needed and tutorials regarding how to do common tasks.

> What is your evaluation of the potential usefulness of the library?

VERY limited.

The library does too little to be useful for real development.
I don't really see a real need for a library that could potentially be
used to create CURL or could potentially be used to write web
framework that users will work over them.

As a user of this library you need to deal with:

- User has to have deep knowledge of HTTP protocol itself and be
highly aware of various security related problems on WWW.
- Protocol itself and know it up to the small details, timeouts and much more.
- Being able to parse all parameters passed from getting query string,
parsing multipart/form-data (a.k.a. file upload format) parse percent
encoding, cookies. I suggest to drawing the line at some point when
library becomes useful for somebody who for examples needs to
implement RESTful service or send some post data to a form.

The separation line that was drawn does not suite the needs of too
many of potential users. I understand it is huge task to do but if
first lines in your FAQ and many other talk about what the library
doesn't do than probably something goes wrong.

And please note I don't talk about sessions, url mapping, but much
more basic stuff like multipart parsing, url-encoding. URL parsing,


> Did you try to use the library? With which compiler(s)? Did you have any problems?

Tried to compile several examples no problem.

> How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?

At least 10-12 hours of reading documentation, code, running some
examples. Notifying potential problems.

> Are you knowledgeable about the problem domain?

Yes, I'm myself an author of CppCMS C++ Web Framework - fully featured
web development platform for C++ that works in both synchronous and
asynchronous modes, I implemented HTTP and FastCGI protocols,
networking engines myself. Designed high performance services and
more.

> Should the library be included to Boost?

Unfortunately NO.

Two major reasons:

- Abuse of static polymorphism where classing object oriented design
with virtual functions can do better job.
- The scope of the library is way too limited that it brings the
question of its usability for real users.

I fully acknowledge the huge high quality work the author did -
something that kept me reviewing the library and providing the
comments, but the library as it is today IMHO doesn't suit Boost

Best Regards and Good Luck,

Artyom Beilis

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

[Peter Dimov via Boost]

This is my review of the Beast library.

I. DESIGN
---------

The design of the library is very good and appears mature. The concepts are
well specified, well documented, well thought out. The use of concept checks
is a nice touch and a mark of professionalism.

I think that basing the library on ASIO (resp. the Networking TS) is a fine
decision. This does mean that if one finds an ASIO/NetTS concept less than
ideal, one has to live with it, but that's an acceptable tradeoff.

Severing the ties with ASIO based on the yet-hypothetical "structured blobs
in, structured blobs out" model may sound good in theory, but in practice, a
supermajority of the potential users of Beast need to produce a working HTTP
or Websocket server, and the library as it stands addresses this need.

The design is not perfect; at times the library makes it much too easy for
asymptomatic mistakes to be introduced by the omission of a required member
function call. For example, if one forgets to set the Content-Length,
everything will still appear to work; or if one forgets to initialize the
.result of the response, it remains uninitialized. (Note that these, and
subsequent, observations appertain to the library as submitted for review;
they may no longer be relevant since the author is very quick with the
fixes.)

There is, at times, unnecessary verbosity, even in the two storefront
examples in

http://vinniefalco.github.io/stage/beast/review/beast/quick_start.html

that are supposed to show the library in the best possible light. For
instance,

// Set up an HTTP GET request message
http::request<http::string_body> req;
req.method(http::verb::get);
req.target("/");
req.version = 11;

Every request will need a method and a target, so having to call members to
set them feels unnecessary. A constructor ought to have been provided.

Similarly, in the next example,

// WebSocket says that to close a connection you have
// to keep reading messages until you receive a close frame.
// Beast delivers the close frame as an error from read.
//
beast::drain_buffer drain; // Throws everything away efficiently
for(;;)
{
// Keep reading messages...
ws.read(drain, ec);

// ...until we get the special error code
if(ec == websocket::error::closed)
break;

// Some other error occurred, report it and exit.
if(ec)
return fail("close", ec);
}

the drain loop is basically a requirement for every correct program, so it
should have been encapsulated into a helper function. (Even though the
example as written does illustrate the use of a drain_buffer.)

I'm not particularly fond of the design decision to declare parts that in
practice every user of the library will need as "out of scope". People
should not be made to reinvent these wheels. I understand the aim of
producing a standard library proposal, and that some of these wheels would
not be suitable for standardization, but there's nothing easier than just
including the suitable wheels in the formal proposal and omitting the
unsuitable ones.

In practice, these necessary parts end up as examples, so one has to
#include "../example/wheel.hpp" instead of "beast/wheel.hpp". It would be
better, in my opinion, if one includes, say, "beast/ext/wheel.hpp", with it
being known that ext/ is where "out of scope" things go.

The ASIO coupling did leave me with one question, whether it would be
possible for the library to accommodate OS-specific file to socket transfers
such as the one provided by TransmitFile:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms740565(v=vs.85).aspx

The idea here is that the OS kernel can dump the file to the socket without
involving user mode, which has the potential of much increased performance.

For the median user, this would not make much of a difference, but it's an
interesting design question, and perhaps an argument that the example
file_body class ought to be part of the library proper. ("Is in scope", to
use the preferred terminology.)


II. IMPLEMENTATION
------------------

I did not pay close attention to the implementation. A random sampling
reveals that it's professionally written, and it appears to work, which is
good enough for me. The tests run when `b2` is invoked in `test`, have good
coverage, and pass with MSVC 14.0 and 14.1.

Travis/Appveyor are put to good use.

The library provides a doc/Jamfile, which is a plus, but this Jamfile fails,
which is a minus. The file reference.qbk has its own separate build script,
and this would need to be fixed one way or another when the library is
integrated into the Boost documentation build.


III. DOCUMENTATION
------------------

The documentation is good. The non-reference parts are very good, the
examples are informative. The reference is very good at times, and not so
much at others. This is caused by the fact that to get to the good parts one
needs to drill down a level or two. For instance, looking at

http://vinniefalco.github.io/stage/beast/review/beast/quickref.html

I noticed the "async_teardown" function in the WebSocket section, so I
clicked on it to see if I need to call that to tear down a WebSocket
connection. This lead me to

http://vinniefalco.github.io/stage/beast/review/beast/ref/beast__websocket__async_teardown.html

which only says "Start tearing down a
boost::asio::ssl::stream/connection/boost::asio::ip::tcp::socket."

Googling told me that I'm not the only one left not entirely satisfied with
these descriptions:

https://github.com/vinniefalco/Beast/issues/274

The confusion here stems from the fact that would I, and the issue
submitter, have clicked on one of the async_teardowns in that page, we would
have reached f.ex.

http://vinniefalco.github.io/stage/beast/review/beast/ref/beast__websocket__async_teardown/overload1.html

which is indeed fine, and mentions that the implementation calls this
function, not the user.

This is a general pattern with the reference. The leaves are perfectly fine,
but the upper levels are often uninformative.

IV. PRACTICAL USE
-----------------

As part of conducting this review, I ported two existing servers of mine to
Beast, one HTTP, one WebSocket. The HTTP server can be seen at

https://github.com/pdimov/beast_tpc_server

and the WebSocket one is at

https://github.com/pdimov/beast_ws_async_server

The experience was pleasant and things went more or less as expected, with
virtually no nasty surprises. The author was very helpful. The library
proper worked exactly as advertised, and the servers were both easier to
write than the originals, and became more robust.

I only wish that more out of scope things (target parser, basic
authorization handling) become in-scope, because their absence is being
felt.

V. VERDICT
----------

Beast should be ACCEPTED. It's useful, it works, it serves a legitimate
need, and I see no design or implementation problems that would preclude
acceptance.

[Vinnie Falco via Boost]

On Mon, Jul 3, 2017 at 4:23 PM, Peter Dimov via Boost
<bo...@lists.boost.org> wrote:
> Every request will need a method and a target, so having to call members to
> set them feels unnecessary. A constructor ought to have been provided.

will fix

> the drain loop is basically a requirement for every correct program, so it
> should have been encapsulated into a helper function. (Even though the
> example as written does illustrate the use of a drain_buffer.)

will fix

> I'm not particularly fond of the design decision to declare parts that in
> practice every user of the library will need as "out of scope".

<beast/http/file_body.hpp> will be a public interface

> The ASIO coupling did leave me with one question, whether it would be
> possible for the library to accommodate OS-specific file to socket transfers
> such as the one provided by TransmitFile:
>
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms740565(v=vs.85).aspx

The answer is: YES.

I have developed a working prototype writing HTTP messages to streams
synchronously and asynchronously using ::TransmitFile. This is
accomplished by providing class file_body_win32 which becomes aliased
to file_body on Windows. Then, overloads of the write functions are
provided which work on messages with the file_body type. These write
functions require boost::asio::basic_stream_socket<Protocol> for the
stream, otherwise ::TransmitFile cannot be used as there is no native
SOCKET object available (the implementation resorts to the regular
write in this case).

This behavior is entirely transparent to users. You can declare:

http::request<file_body> req;

Then later, call:

http::async_write(stream, req, [](error_code ec){});

and everything Just Works. Windows uses ::TransmitFile and other
platforms use the file_body_stdc (which uses ugly FILE* but works).
This technique can be extended to interfaces specific to other
platforms. The prototype is available here:
<https://github.com/vinniefalco/Beast/commits/review-addendum-1>

Thanks

[Marcel Ebmer via Boost]

Here's my Review:

DESIGN
======
The design of concepts, data structures and algorithms is mostly done well, with
only minor concerns:
- The smell of two-phase initialization.
Required message properties like method and target have to be set from
outside. This makes me as a library user afraid of forgetting something
vital. The same holds for prepare_payload(). If I forget to call
prepare_payload() or forget to set one of the required message properties or
fields, my program may - in the worst case - appear to work correctly.
- Inconsequent application of automagic.
WebSocket control frames are automagically responded to. Iff there's an
active read(). Basically, having an active read() at almost all times is a
requirement of any correct program using Beast. Even though I enjoy the help
from the library, and the requirement is usually met, I found this specific
behaviour to be inconsequent.
Also, the very first WebSocket example in the docs makes it clear that while
a close from the peer is handled automagically under the above condition, a
close from my side is quite tedious to write. There should be a utility in
the library to do the draining for me.
- The naming of BodyReader and BodyWriter initially got me confused.
In my mind, I 'read' a body FROM a stream/buffer, I 'write' it INTO a stream
or buffer. The library uses the terms in the reverse sense. This is not
incorrect of course, but my initial confusion may be a sign of suboptimal
naming.

IMPLEMENTATION
==============
Didn't look in-depth.

DOCUMENTATION
=============
The documentation is very well structured. It is complete in many, but not all
places.
- A minimalistic HTTP server example would have helped me get started. Just
something very, very basic. Reply to each request with "your request was".
- In some places, non-self-explanatory function names suddenly pop up
without being first introduced. For example, in the section
"Using WebSocket"."Send and Receive Messages", text() and got_text() could
use some explanation. In the same section, the reference to set_option()
seems out-of-place and makes me wonder what I missed.
- Since the documentation features comparison with other libraries, it may
be worth comparing pion (https://github.com/splunk/pion) as well.
- Many reference pages could use some prosaic explanation. For example, my
uncertainty concerning message.prepare_payload() might have been alleviated,
were it more clear what the function is intended for and what the effect of
omitting it would be.

SCOPE
=====
Since the FAQ is quite defensive in regard to the library's scope, I feel this
is worth some discussion.
I found the scope to be just right for now, but with future potential.
As it is, the library takes away all the tedious protocol stuff, while leaving
all important design decisions to the user. It feels like an extension to ASIO.
However, there's no apparent reason why (e.g.) request targets shouldn't be
URL-decoded, and why form data should not be parsed into a map of some kind. The
way I see it, none of this added funtionality would interfere with the library's
design. It would, however, make the library user's life a whole lot easier, and
probably attract more users as well.

USEFULNESS
==========
See SCOPE. Very valuable, could be even better.

HOW MUCH EFFORT & TRIED TO USE
==============================
I invested about a day into the review, read the documentation and ported a
small REST-like service from pion to Beast. I may be biased towards Beast due to
temporal proximity, but I feel the latter made the code easier to reason about
and, at the very least, did not hurt performance. I used gcc 7.1 and clang 4.0.
On the review branch, none of the two had any issues.
I deliberately did NOT follow the discussion on the mailing list, in order to
remain as unbiased as practicable.

HOW KNOWLEDGABLE AM I?
======================
I have worked on several HTTP servers and experimented with writing an HTTP
server library a few years back. I have no practical experience with WebSockets.

IN ONE SENTENCE
===============
Beast should be ACCEPTED into Boost.

Marcel

[Vinnie Falco via Boost]

On Wed, Jul 5, 2017 at 2:31 AM, Marcel Ebmer via Boost
<bo...@lists.boost.org> wrote:
> Here's my Review:

Thanks you!

> Required message properties like method and target have to be set from
> outside. This makes me as a library user afraid of forgetting something
> vital.

message will have constructor variations for setting the header values
<https://github.com/vinniefalco/Beast/commit/8d677757645405295a1417b703c19f2d4f71d20c>

> The same holds for prepare_payload(). If I forget to call
> prepare_payload() or forget to set one of the required message
> properties or fields

> WebSocket control frames are automagically responded to. Iff there's an
> active read(). Basically, having an active read() at almost all times is a
> requirement of any correct program using Beast. Even though I enjoy the
> help from the library, and the requirement is usually met, I found this
> specific behaviour to be inconsequent.

What's the call to action? What change is being proposed?

> There should be a utility in the library to do the draining for me.

willfix
<https://github.com/vinniefalco/Beast/issues/564>

> However, there's no apparent reason why (e.g.) request targets shouldn't be
> URL-decoded, and why form data should not be parsed into a map of some kind.
> The way I see it, none of this added funtionality would interfere with the
> library's design.

"out of scope": that dreaded phrase that no one wants to hear :)

The features you described are arbitrary. Why forms and not basic
authentication? Why URL decoding but not a full URI parser? Why a full
URI parser but no handling for redirects? What about supporting
proxies? Anything that I add is going to satisfy some users and
disappoint others.

That's why I'd like to keep Beast as it is, a low level protocol
layering on top of Asio. I am carefully and strategically adding
things to Beast where it makes sense. For example, based on feedback I
am making file_body a public interface. Taking advantage of platform
specific optimizations to send files as HTTP message payloads is most
certainly in scope.

Dealing with the actual content of the message, is not - subject to
the caveat that Beast must understand enough of the message to
reliably get it from one end to the other. That's why I could accept
the argument that supporting alternate status-lines (e.g. "ICY 200
OK\r\n") should be in-scope.

That doesn't mean that I won't be building new higher level features
(I'm already working on a URL decoder) but I think they belong in a
separate library with its own documentation, tests, CI infrastructure,
and also separate Boost formal review process. Still, there's only so
much I can do - HTTP is a MASSIVE domain!. The only reason I was able
to get Beast to where it is at was by being strict on what features I
would support.

I'm hopeful that other people will build on top of Beast and propose
their own new libraries to fill in these gaps.

Every so often, students or interns come to the mailing list and ask
how they can help. Here's an idea, port this library to modern C++ and
Beast:
<http://www.codesink.org/mimetic_mime_library.html>

Thanks

[Marcel Ebmer via Boost]

On 07/05/2017 04:13 PM, Vinnie Falco wrote:
>> WebSocket control frames are automagically responded to. Iff there's an
>> active read(). Basically, having an active read() at almost all times is a
>> requirement of any correct program using Beast. Even though I enjoy the
>> help from the library, and the requirement is usually met, I found this
>> specific behaviour to be inconsequent.
> What's the call to action? What change is being proposed?

I don't have a concrete proposal. What I was trying to describe is the
discrepancy between the general look and feel of the library - where the
user is in full control, and hence bears full responsibility - and the
automagic happening with WebSocket control frames. And, to make it feel
even more awkward, the automagic is actually semi-automatic. I realize
there's probably no sane way to have control frames always responded to
during the lifetime of a websocket::stream. It may, however, be
reasonable to give control back to the user and just provide convenient
utilities for sending the appropriate responses.

> "out of scope": that dreaded phrase that no one wants to hear :)
>
> The features you described are arbitrary. Why forms and not basic
> authentication? Why URL decoding but not a full URI parser? Why a full
> URI parser but no handling for redirects? What about supporting
> proxies? Anything that I add is going to satisfy some users and
> disappoint others.

I don't agree. Any feature you add will satisfy some users, and
not-yet-satisfy others. Big difference.

> That doesn't mean that I won't be building new higher level features
> (I'm already working on a URL decoder) but I think they belong in a
> separate library with its own documentation, tests, CI infrastructure,
> and also separate Boost formal review process. Still, there's only so
> much I can do - HTTP is a MASSIVE domain!. The only reason I was able
> to get Beast to where it is at was by being strict on what features I
> would support.

For a new library, the expectations would be very high. It would have to
support all of HTTP - which may well mean that such a swiss army knife
library will never happen. For an existing library, like Beast, the
level of expectation (concerning new features) is a lot more bearable.
Anything you give us will be seen as a goodie. Beast CAN add an URI
parser without supporting Basic Authentification. Or Basic Auth without
supporting form data at the same time. You could also put the
functionality into a utility namespace and exclude that from your
standards proposal.

[Vinnie Falco via Boost]

On Fri, Jul 7, 2017 at 3:37 AM, Marcel Ebmer via Boost
<bo...@lists.boost.org> wrote:
>....

\sigh, yeah yeah. Point taken! I will consider adding some things.

[Gyorgy Szekely via Boost]

Hi,
Some background: this is my first Boost library review. I followed the
discussion on Beast and I'm probably not as experienced as others on this
topic. I started learning Beast along with Asio and it was a really good
experience. After understanding ASIO concepts, I could get an async
multi-threaded http server running in a few hours.
I leave it up to the review manager whether to take this a formal review or
just informal feedback (impressions of newbie :))


*** What is your evaluation of the design?
The library in my opinion is very well designed. I doesn't try to offer too
much. I understand the author's position: HTTP is vast and Beast is a
low-level library. Still, while using it I felt with a few additional bits
it would be more complete. What I was missing is parsers/serializers for
uri and cookies. I think Beast should provide these (even in a separate
namespace), as they are not higher level functionality than
parsing/serializing http headers.

(I implemented them in my application, and I'm sure my code is neither
correct regarding corner cases, nor standards compliant. I'm also sure that
I'm not alone with this, so there's a place for this in either Beast, or as
the author suggests a higher level lib.)

Though the library is heavily templated, as a user I'm not forced to write
templated code, implementing the most common use-cases is simple and
straightforward. That's a big plus.

I really like the symmetric client/server design. My application is
http/websocket server, being able to use the the same classes and functions
in unit tests made reasoning about the code and tests much easier.

Finally there's was a lot of debate whether being ASIO and stream concept
based is a good idea or not. For me (as a newcomer) seamless integration
with ASIO helped getting started really fast, though I didn't do awfully
complex stuff.


*** What is your evaluation of the implementation?
I did not review all the implementation. While using the library, I looked
into some of the files, and my impression was that the code is high
quality. The test coverage is also high, and issue fixes are accompanied
with rigorous tests.
The library is mostly composed of templates, which makes a bit harder to
understand its internals, but I didn't mind it at all given the benefits
(composablity, customization points).


*** What is your evaluation of the documentation?
The documentation is well structured and plenty. Sections from the Intro up
to the Reference are a good read by themselves. Design decisions and
library scope are clearly stated. There're lots of examples in form of code
snipets highlighting a particular use-case, as well as fully functional
programs demonstrating these in action.
I liked that the documentation is similar to ASIO's and I could use them
together.


*** What is your evaluation of the potential usefulness of the library?
It's super useful. Even with its (current) dependency on ASIO and some
other Boost libs Beast is small while providing both client and server side
foundations for Websocket and HTTP. NodeJS and Java makes it so simple to
create REST services, this should be hard in C++ either with standards
based (NetTS) solutions. Beast is a step in the right direction.


*** Did you try to use the library? With which compiler(s)? Did you have
any problems?
I'm actively using the library in the Http/Websocket <-> ZeroMQ gateway
since Oct 2016 on Ubuntu 16.04 with GCC5, C++14. We've run extensive
performance and stability tests on the HTTP interface and we're quite
satisfied with the results. I migrated our codebase to newer Beast versions
multiple times, and during this I reported a few minor issues or requested
new features. The library author was really responsive and
fixed/implemented them quickly.


*** How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?
A few hours this time, but I've read the documentation and the public API
reference multiple times in the last months. I didn't go through the
implementation, but peeked into it here and there to understand some of the
details.


*** Are you knowledgeable about the problem domain?
Not really. I've implemented some NodeJS and Java Spring REST services, but
that's all.


My conclusion: Beast should be ACCEPTED.


Regards,
Gyorgy Szekely

[Jens Weller via Boost]

This is my review of the beast libary.

First, I find the name quite fitting, but also we clearly need a "cage" library to make the internals of beast more useful for the endusers.
I spend a few days on and off reviewing examples, and documentation. I used the library as an example for a fuzzing experiment on the weekend.

What is your evaluation of the design?

The design is very boost like, the library fits well into boost.
One thing the library shares with asio is the buffer concept, which I never really liked.
Buffer feels to me to abstract, to far away from the usual type vocabulary one is used to from standard C++.
But once you're used to the concept, its ok. So no big deal.

The dependence on Asio was already mentioned during the review and I am not sure about this.
Surely a nice goal to have, but also not an easy one.
But replacing asio with an abstraction that allows for pluggin in other socket implementations would be very powerful.

Its nice that the library is header only, but over asio you get the dependency to boost::system for error_code.
I'd love to be able to exchange boost::error_code dependency to the std::error_code one.
As the beauty of header only really shines when you don't need to link to other non header only libraries...

What is your evaluation of the implementation?

Fuzzing. I spend this weekend some time to fuzz beast with libFuzzer. The basic_parser and the websocket::stream were fuzzed.
A bug (buffer overflow) in basic_parser was found, and is already fixed.

What is your evaluation of the documentation?

Boost typical. Its good, helps with getting the library to know.

What is your evaluation of the potential usefulness of the library?

This library could be the foundation of many things to come. The internet is the basic building block for so much today, that beast is a nice improvement upon asio in providing the vocabulary to use boost in the internet age.

As I already mentioned, I think that other libraries are needed in order to make this library really useful for most end users.
But the scope the author aims at, beast fits very well.

Did you try to use the library? With what compiler? Did you have any problems?

Fuzzing was based on clang 5.0, but I also used the library with GCC 5.4 to generate example data for the fuzzing.
I plan to use the library to write a small http server and clients for my own usage.

How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?

My time for this review is limited, but as the author mentioned on this list, that the library hasn't been fuzzed, I aimed for doing this instead of implementing an http client/server.
I still had to write code in order to reach the parts in beast to be fuzzed, this gave a more detailed tour of beast, then one would need for simple example exploring.

Are you knowledgeable about the problem domain?

A little, I am mostly a high level user of various HTTP libraries, but not at the low level beast covers.
I found it interesting to get a glance at the details.

And finally, every review should answer this question:

Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion.

I think that this library should be ACCEPTED into boost. It is an important improvement on asio, in order to offer a more detailed networking stack in boost.
The library has the exact right scope to do this, widening the scope into providing client/server implementations would not be good. I see this in another library.

thanks,

Jens Weller

[Vinnie Falco via Boost]

On Sun, Jul 9, 2017 at 11:29 AM, Jens Weller via Boost
<bo...@lists.boost.org> wrote:
> But replacing asio with an abstraction that allows for pluggin in other socket
> implementations would be very powerful.

Fortunately we will have that soon! Its called "Networking-TS". And
Beast will use it (either through an upgrade of the library or through
a port in a separate library).

> I'd love to be able to exchange boost::error_code dependency to the std::error_code

If Boost.Asio supported that, then Beast would too. But since Asio
doesn't support it, Beast can't support it either. The problem is that
the SyncReadStream, SyncWriteStream, ReadHandler, and WriteHandler
concepts prescribe a specific type (boost::system::error_code).

I am sure that this problem will be resolved. Its already fixed in
Networking-TS. If Boost.Asio is updated to include changes to make
std::error_code work, I will update Beast right away!

> I spend this weekend some time to fuzz beast with libFuzzer. The basic_parser and
> the websocket::stream were fuzzed.
> A bug (buffer overflow) in basic_parser was found, and is already fixed.

This was great, your fuzzer output was really quite detailed! It
included the specimen which caused the crash, a nice easy to read
stack trace, and a report from address-sanitizer explaining the
problem. Once I got the specimen into a unit test the problem became
trivial to fix.

> I think that other libraries are needed in order to make this library really useful for
> most end users.

I agree!

Thanks

[Phil Endecott via Boost]

Jens Weller wrote:
> Fuzzing. I spend this weekend some time to fuzz beast with libFuzzer.
> The basic_parser and the websocket::stream were fuzzed.
> A bug (buffer overflow) in basic_parser was found, and is already fixed.

*THANK YOU* so much for doing that. I didn't see your message until
after I'd sent my review, and I feel even more justified in my comments
about the over-complex optimisations in the parser, and the security
implications.

I'd be interested to see where the bug was. Was this posted on the list?


Regards, Phil.

[Vinnie Falco via Boost]

On Sun, Jul 9, 2017 at 11:57 AM, Phil Endecott via Boost
<bo...@lists.boost.org> wrote:
> I feel even more justified in my comments
> about the over-complex optimisations in the parser, and the security
> implications.

Of course I completely disagree with your points, especially the one
implying that I wasted time optimizing instead of adding features you
thought were necessary. But nothing wrong with expressing them.

> I'd be interested to see where the bug was. Was this posted on the list?

No, I will post it.

Thanks

[Jens Weller via Boost]

> Gesendet: Sonntag, 09. Juli 2017 um 20:57 Uhr
> Von: "Phil Endecott via Boost" <bo...@lists.boost.org>
> An: bo...@lists.boost.org
> Cc: "Phil Endecott" <spam_from...@chezphil.org>
> Betreff: Re: [boost] [beast] Review

>
> Jens Weller wrote:
> > Fuzzing. I spend this weekend some time to fuzz beast with libFuzzer.
> > The basic_parser and the websocket::stream were fuzzed.
> > A bug (buffer overflow) in basic_parser was found, and is already fixed.
>
> *THANK YOU* so much for doing that. I didn't see your message until
> after I'd sent my review, and I feel even more justified in my comments
> about the over-complex optimisations in the parser, and the security
> implications.
>
> I'd be interested to see where the bug was. Was this posted on the list?

I used beast to get into fuzzing with this workshop:
https://github.com/Dor1s/libfuzzer-workshop

Motivation was that in that way I could contribute to the review and learn something non beast related.
TWO things at once! The fuzzer found the bug pretty fast, almost instantly.

I'm not a fuzzing expert, but I as far as I know I got lucky with an oversight in the handling of results in the beast parser, it appears.

I continued the fuzzing of beast after vinnie provided a fix and so I also fuzzed this branch.
Nothing else came up.

thanks,

Jens Weller