Skip to content

Conversation

@MarekKnapek
Copy link

Avoid footgun.

Factor temporary std::string instance to separate variable
as this prolongs its lifetime long enough for result of
.c_str to survive. Otherwise printf could obtain pointer
to already destructed string.

For more information consult GotW #88 at [1].

[1] https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/

Use {} instead of () for object construction.

Sometimes it is difficult to distinguish between
a function call and object construction. It is better
to use {} for object construction for this reason.

std::unique_ptr baby

Captures in lambdas are const by default. So in order to
be able to move from them, we mark the entire lambda
as mutable. Thus, member variables in our closure object
are no longer const. Also, using a variable multiple times
on the same line and one of the uses is modifying the
variable is a no-no. Extract the pointer out of the unique_ptr
before performing modifying operation.

Factor temporary std::string instance to separate variable
as this prolongs its lifetime long enough for result of
.c_str to survive. Otherwise printf could obtain pointer
to already destructed string.

For more information consult GotW #88 at [1].

[1] https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/
Sometimes it is difficult to distinguish between
a function call and object construction. It is better
to use {} for object construction for this reason.
Captures in lambdas are const by default. So in order to
be able to move from them, we mark the entire lambda
as mutable. Thus, member variables in our closure object
are no longer const. Also, using a variable multiple times
on the same line and one of the uses is modifying the
variable is a no-no. Extract the pointer out of the unique_ptr
before performing modifying operation.
@oschonrock
Copy link

oschonrock commented Oct 8, 2025

I see what you are getting at however...

error.message().c_str()

is passed immediately to printf(). In C++, the temporary std::string created by error.message() lives until the end of the full expression, which is the call to printf.

Therefore, the std::string returned by message() remains valid for the duration of the printf call, and its .c_str() is safe to use in this context.

Agree?

@oschonrock
Copy link

fully agree with the std::unique_ptr solution

it is much easier to see the problem when using -fsanitze=adress
as this shows us which null ptr is being dereferenced

@oschonrock
Copy link

oschonrock commented Oct 8, 2025

there is another bug, need to restrict number of bytes written to number of bytes read:

eg like this

        auto req_ptr2 = request.get(); // need to cache ptr to prevent segfault (arg eval order)
        asio::async_write(req_ptr2->socket, asio::buffer(req_ptr2->buffer.data(), bytes_transferred), ... 

I noticed the problem when using this for the client:

netcat localhost 6970 < /dev/random

in that case the bytes sent is always the full buffer when the bytes read is much less.. so it was sending a lot of garbage

@MarekKnapek
Copy link
Author

Oh, my bad, in this case the temporary string survives long enough. But in general, this it gotcha, the function might store the pointer for later use.

About lambdas and unique pointers. Yeah, sad story. It might be better to capture plain old pointer instead and avoid the mutable keyword. Using .release() on a line just before the capture and constructing fresh new unique pointer on first line inside the lambda passing the plain pointer as an argument. Might not be idiomatic C++ but readable and understandable by a C professional.

@oschonrock
Copy link

Oh, my bad, in this case the temporary string survives long enough. But in general, this it gotcha, the function might store the pointer for later use.

About lambdas and unique pointers. Yeah, sad story. It might be better to capture plain old pointer instead and avoid the mutable keyword. Using .release() on a line just before the capture and constructing fresh new unique pointer on first line inside the lambda passing the plain pointer as an argument. Might not be idiomatic C++ but readable and understandable by a C professional.

yeah fully agree..
actually shared_ptr is very commonly used for this stuff, the costs are tiny compared with the costs and performance of the network stack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants