Beast: buffer overflow handling for string_body and mutable_body

[octopus-prime]

Beast/v70

Was looking at test coverage to find untested parts to increase coverage.
So i discovered this:

            try
            {
                body_.resize(len + n);
            }
            catch(std::length_error const&)
            {
                ec = error::buffer_overflow;
                return;
            }
            ec.assign(0, ec.category());

There you catch std::length_error only.
What about the exceptions thrown by corresponding Allocator?
e.g. std::bad_alloc

http://en.cppreference.com/w/cpp/string/basic_string/resize

[vinniefalco]

They are not caught!

[octopus-prime]

Is this good or bad?

[vinniefalco]

I think its bad, what do you propose for the fix? What exceptions should it catch, and what errors should it map them to?

[octopus-prime]

I would suggest catching std::exception and always mapping to error::buffer_overflow.

[vinniefalco]

I'm okay with that, would you like to submit a pull request? mutable_body needs it too (I think)

[octopus-prime]

Yes mutable_body has code duplicated from string_body, since Reader and Writer are exactly the same... So they should use the same Reader/Writer in future?!

I can do the fixes and making a pull request.

What about this:

            if(content_length)
            {
                if(*content_length > (std::numeric_limits<std::size_t>::max)())
                {
                    ec = make_error_code(errc::not_enough_memory);
                    return;
                }
                ec.assign(0, ec.category());
                body_.reserve(static_cast<std::size_t>(*content_length));
            }
            else
            {
                ec.assign(0, ec.category());
            }

There is no try/catch at all.

[vinniefalco]

So they should use the same Reader/Writer in future?!

If you mean that string_body should include the header file for mutable_body or vice versa, no, because library code cannot use code from example/. It has to be duplicated. I think that's okay.

There is no try/catch at all.

Looks like another area that needs try and catch

[vinniefalco]

By the way, these are the kinds of bugs we like, easy to fix and doesn't change any public interfaces. As long as there is no major design problem with interfaces, then we're in good shape - things like this can be cleaned up over time (not saying we shouldn't fix it now but this is not as severe as an interface design flaw).

[octopus-prime]

So you create a new branch first?

[vinniefalco]

Yeah just make a new branch in your fork, based off v70 and place the commit which edits both classes (don't forget the CHANGELOG.md)

[octopus-prime]

v70 is master now, isn't it?
I think i merged...

[vinniefalco]

Yeah it merged, I just updated the docs. No worries! Maybe its time to create a develop branch.. heh

[vinniefalco]

Yes, we need a develop branch because I am not supposed to make changes to master during the formal review. I will create it now.

[octopus-prime]

So i should wait for develop branch to base on it?

[vinniefalco]

So i should wait for develop branch to base on it?

No, because the develop branch will be identical to master right now. Just make a new branch in your fork, based on master (which is v70).

[octopus-prime]

Is it okay if all problems with length are mapped to
ec = error::buffer_overflow;
?

[octopus-prime]

        template<bool isRequest, class Fields>
        explicit
        writer(message<isRequest, string_body, Fields>& m,
            boost::optional<std::uint64_t> content_length,
                error_code& ec)
            : body_(m.body)
        {
            if(content_length)
            {
                if(*content_length > (std::numeric_limits<
                        std::size_t>::max)())
                {
                    ec = error::buffer_overflow;
                    return;
                }
                try
                {
                    body_.reserve(static_cast<
                        std::size_t>(*content_length));
                }
                catch(std::exception const&)
                {
                    ec = error::buffer_overflow;
                    return;
                }
            }
            ec.assign(0, ec.category());
        }

[vinniefalco]

I can't think of anything wrong with that. Its better than not catching the exception. And we can always change the error later if we want something better.

[octopus-prime]

Is "Fix buffer overflow handling for string_body" as commit message okay?

[vinniefalco]

Yes that's great

[octopus-prime]

okay. would like to try again. bugfix based on which branch?

[vinniefalco]

develop

[octopus-prime]

Headline for CHANGELOG.md in bugfix?
Currently it is "Version 70"...

[vinniefalco]

Version 71:

--------------------------------------------------------------------------------

Version 70:
...

[octopus-prime]

"Version 71"? I'm asking because there is a v71 branch...

[vinniefalco]

Its fine, when your pull request passes tests I will just cherry-pick your commit into the v71 branch.