[beast] Review

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[beast] Review

Boost - Dev mailing list
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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [beast] Review

Boost - Dev mailing list
On Sun, Jul 9, 2017 at 11:29 AM, Jens Weller via Boost
<[hidden email]> 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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [beast] Review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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.





_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [beast] Review

Boost - Dev mailing list
On Sun, Jul 9, 2017 at 11:57 AM, Phil Endecott via Boost
<[hidden email]> 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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [beast] Review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list


> Gesendet: Sonntag, 09. Juli 2017 um 20:57 Uhr
> Von: "Phil Endecott via Boost" <[hidden email]>
> An: [hidden email]
> Cc: "Phil Endecott" <[hidden email]>
> 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

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