Skip to content

Fix stale async handler after timeout - #224

Open
mr-j0nes wants to merge 4 commits into
karastojko:masterfrom
mr-j0nes:master
Open

Fix stale async handler after timeout#224
mr-j0nes wants to merge 4 commits into
karastojko:masterfrom
mr-j0nes:master

Conversation

@mr-j0nes

Copy link
Copy Markdown

Cancel socket and drain pending handlers before throwing in wait_async(), preventing use-after-free when dialog is copied

Fix: #223, maybe #222 too

Cancel socket and drain pending handlers before throwing in
wait_async(), preventing use-after-free when dialog is copied

Fix: karastojko#223, maybe karastojko#222 too

@Spixmaster Spixmaster left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CMake issues are critical. Currently, the tests do not even build.

The other stuff is from clang-tidy. You should really use it.

Comment thread test/CMakeLists.txt Outdated
Comment thread test/CMakeLists.txt Outdated
Comment thread test/test_dialog_stale_handler.cpp Outdated
Comment thread test/test_dialog_stale_handler.cpp Outdated
Comment thread test/test_dialog_stale_handler.cpp
Comment thread test/test_dialog_stale_handler.cpp Outdated
Comment thread test/test_dialog_stale_handler.cpp Outdated
Comment thread test/test_dialog_stale_handler.cpp
Comment thread test/test_dialog_stale_handler.cpp
Comment thread test/test_dialog_stale_handler.cpp
@Spixmaster

Copy link
Copy Markdown
Contributor

Tests are passing on my side.

@Spixmaster

Copy link
Copy Markdown
Contributor

Issue #222 was not fixed by this.

- test/CMakeLists.txt: remove unnecessary Boost::system dependency (karastojko#212)
- Add explicit on TestServer constructor
- Add missing includes: unit_test_suite, tools/old/interface, steady_timer,
  system/error_code, string
- Use explicit curly braces on all if statements
- Fix unused return value from socket_->close(ec)
@mr-j0nes

Copy link
Copy Markdown
Author

All issues in comments have been resolved. Let me know if I missed anything..

Comment thread test/test_dialog_stale_handler.cpp Outdated
@@ -12,12 +12,17 @@ copy at http://www.freebsd.org/copyright/freebsd-license.html.
#define BOOST_TEST_MODULE dialog_test

#include <boost/test/unit_test.hpp>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#include <boost/test/unit_test.hpp> is not needed.

Comment thread test/test_dialog_stale_handler.cpp Outdated
Comment thread test/test_dialog_stale_handler.cpp Outdated
Comment thread test/test_dialog_stale_handler.cpp Outdated
Comment thread test/test_dialog_stale_handler.cpp Outdated
@Spixmaster

Copy link
Copy Markdown
Contributor

Thank you very much. You can mark the recommendations as resolved when you considered them to distinguish between open and closed recommendations.

- Remove unused `<boost/asio.hpp>`, add explicit includes
- Add missing `<chrono>`, `<string>`, `<cstddef>`, `<boost/assert.hpp>`
- Add `const` to `error_code` lambda parameters
- Add `explicit` on `TestServer` constructor
- Use explicit curly braces on all if statements
- Check `socket_->close()` error code with `BOOST_CHECK` instead of
@mr-j0nes

Copy link
Copy Markdown
Author

Ok, just pushed a new commit with all new comments addressed.
Out of curiosity, how do you run clang-tidy? I have installed it and run it on the test and got nothing.

clang-tidy --version
Ubuntu LLVM version 14.0.0
  
  Optimized build.
  Default target: x86_64-pc-linux-gnu
  Host CPU: tigerlake

@Spixmaster

Spixmaster commented May 25, 2026

Copy link
Copy Markdown
Contributor

I depends on the configuration file.

--config-file=~/clang/clang-tidy.yaml

This is my current clang-tidy.yaml.

---
# "google-readability-namespace-comments" needs to be removed as it is an alias for "llvm-namespace-comment" and
# thus prevents fixing.
Checks: "*, -abseil-string-find-str-contains, -altera-id-dependent-backward-branch, -altera-unroll-loops, -bugprone-branch-clone,
  -bugprone-easily-swappable-parameters, -bugprone-suspicious-missing-comma, -bugprone-unused-raii, -cert-err58-cpp, -cppcoreguidelines-avoid-magic-numbers,
  -fuchsia-default-arguments-calls, -fuchsia-default-arguments-declarations, -fuchsia-overloaded-operator, -fuchsia-statically-constructed-objects,
  -google-readability-function-size, -google-readability-namespace-comments, -google-readability-todo, -hicpp-function-size,
  -hicpp-named-parameter, -hicpp-use-auto, -llvm-else-after-return, -llvm-include-order, -llvmlibc-callee-namespace, -llvmlibc-implementation-in-namespace,
  -llvmlibc-inline-function-decl, -llvmlibc-restrict-system-libc-headers, -misc-no-recursion, -modernize-loop-convert, -modernize-use-auto,
  -modernize-use-designated-initializers, -modernize-use-nodiscard, -modernize-use-trailing-return-type, -openmp-exception-escape,
  -performance-inefficient-string-concatenation, -readability-else-after-return, -readability-function-cognitive-complexity,
  -readability-function-size, -readability-identifier-length, -readability-magic-numbers, -readability-named-parameter"
FormatStyle: ${XDG_CONFIG_HOME}/clang/clang-format.yaml

In general, it is useful to integrate linter (gives recommendations) and formatter into your editor. For me, it is Vim. You might be interested in at least the tools here, https://github.com/dense-analysis/ale/blob/master/supported-tools.md, or even the programme itself, https://github.com/dense-analysis/ale.

clang-format is my favourite formatter for C++.

@Spixmaster Spixmaster left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type of socket_->cancel(ec); in src/dialog.cpp should really be addressed.

Comment thread src/dialog.cpp Outdated
Comment thread src/dialog.cpp
Comment thread src/dialog.cpp
Comment thread src/dialog.cpp Outdated
Comment thread test/test_dialog_stale_handler.cpp
Comment thread test/test_dialog_stale_handler.cpp Outdated
@Spixmaster

Copy link
Copy Markdown
Contributor

I just tested this pull request.

normal mailio: my server can send e-mail, my computer cannot

this pull request: My computer sent one e-mail. I did not test my server.

A good first sign.

- src/dialog.cpp: suppress bugprone-unused-return-value for
  socket_->cancel() calls by assigning to std::ignore
- test/test_dialog_stale_handler.cpp: use return value of
  socket_->close() in BOOST_CHECK instead of out-param
@mr-j0nes

Copy link
Copy Markdown
Author

I am glad. Sent out the fix for the latest comments. Some of them where false positive though. The ec value cannot be defined as const as cancel() accepts a reference. I used std::ignore() to silence "The value returned by this function should not be disregarded;", as we are throwing anyway..

@mr-j0nes

Copy link
Copy Markdown
Author

I depends on the configuration file.

--config-file=~/clang/clang-tidy.yaml

This is my current clang-tidy.yaml.

---
# "google-readability-namespace-comments" needs to be removed as it is an alias for "llvm-namespace-comment" and
# thus prevents fixing.
Checks: "*, -abseil-string-find-str-contains, -altera-id-dependent-backward-branch, -altera-unroll-loops, -bugprone-branch-clone,
  -bugprone-easily-swappable-parameters, -bugprone-suspicious-missing-comma, -bugprone-unused-raii, -cert-err58-cpp, -cppcoreguidelines-avoid-magic-numbers,
  -fuchsia-default-arguments-calls, -fuchsia-default-arguments-declarations, -fuchsia-overloaded-operator, -fuchsia-statically-constructed-objects,
  -google-readability-function-size, -google-readability-namespace-comments, -google-readability-todo, -hicpp-function-size,
  -hicpp-named-parameter, -hicpp-use-auto, -llvm-else-after-return, -llvm-include-order, -llvmlibc-callee-namespace, -llvmlibc-implementation-in-namespace,
  -llvmlibc-inline-function-decl, -llvmlibc-restrict-system-libc-headers, -misc-no-recursion, -modernize-loop-convert, -modernize-use-auto,
  -modernize-use-designated-initializers, -modernize-use-nodiscard, -modernize-use-trailing-return-type, -openmp-exception-escape,
  -performance-inefficient-string-concatenation, -readability-else-after-return, -readability-function-cognitive-complexity,
  -readability-function-size, -readability-identifier-length, -readability-magic-numbers, -readability-named-parameter"
FormatStyle: ${XDG_CONFIG_HOME}/clang/clang-format.yaml

In general, it is useful to integrate linter (gives recommendations) and formatter into your editor. For me, it is Vim. You might be interested in at least the tools here, https://github.com/dense-analysis/ale/blob/master/supported-tools.md, or even the programme itself, https://github.com/dense-analysis/ale.

clang-format is my favourite formatter for C++.

It would be good to include into the repo your clang-tidy.yaml and clang-format.yaml, to prevent going back and forward with PRs

@Spixmaster

Copy link
Copy Markdown
Contributor

I depends on the configuration file.
--config-file=~/clang/clang-tidy.yaml
This is my current clang-tidy.yaml.

---
# "google-readability-namespace-comments" needs to be removed as it is an alias for "llvm-namespace-comment" and
# thus prevents fixing.
Checks: "*, -abseil-string-find-str-contains, -altera-id-dependent-backward-branch, -altera-unroll-loops, -bugprone-branch-clone,
  -bugprone-easily-swappable-parameters, -bugprone-suspicious-missing-comma, -bugprone-unused-raii, -cert-err58-cpp, -cppcoreguidelines-avoid-magic-numbers,
  -fuchsia-default-arguments-calls, -fuchsia-default-arguments-declarations, -fuchsia-overloaded-operator, -fuchsia-statically-constructed-objects,
  -google-readability-function-size, -google-readability-namespace-comments, -google-readability-todo, -hicpp-function-size,
  -hicpp-named-parameter, -hicpp-use-auto, -llvm-else-after-return, -llvm-include-order, -llvmlibc-callee-namespace, -llvmlibc-implementation-in-namespace,
  -llvmlibc-inline-function-decl, -llvmlibc-restrict-system-libc-headers, -misc-no-recursion, -modernize-loop-convert, -modernize-use-auto,
  -modernize-use-designated-initializers, -modernize-use-nodiscard, -modernize-use-trailing-return-type, -openmp-exception-escape,
  -performance-inefficient-string-concatenation, -readability-else-after-return, -readability-function-cognitive-complexity,
  -readability-function-size, -readability-identifier-length, -readability-magic-numbers, -readability-named-parameter"
FormatStyle: ${XDG_CONFIG_HOME}/clang/clang-format.yaml

In general, it is useful to integrate linter (gives recommendations) and formatter into your editor. For me, it is Vim. You might be interested in at least the tools here, https://github.com/dense-analysis/ale/blob/master/supported-tools.md, or even the programme itself, https://github.com/dense-analysis/ale.
clang-format is my favourite formatter for C++.

It would be good to include into the repo your clang-tidy.yaml and clang-format.yaml, to prevent going back and forward with PRs

I have not authority over this project. Linter and formatter are something the owner decides.

@Spixmaster

Copy link
Copy Markdown
Contributor

I have never seen std::ignore. Good job. Indeed, clang-tidy has a false positive regarding boost::system::error_code ec.

@karastojko

Copy link
Copy Markdown
Owner

The 0.26 is released, so I will take a look of this. Thanks for the effort.

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.

Stale async handler crash in timeout + dialog copy scenario

3 participants