[RFC,v2] apps: cam: Use signalfd

24164 diff mbox series

Use a signalfd to detect `SIGINT` instead of registering a signal handler
in order to remove the `CamApp` singleton. This is better than using a
normal signal handler: a signal can arrive after the `CamApp` object has
been destroyed, leading to a use-after-free. Modifying the `CamApp::app_`
in the destructor is not a good alternative because that would not be async
signal safe (unless it is made atomic).

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
changes in v2:
	* move into `CamApp::init()`
	* use `pthread_sigmask()` instead of `sigprocmask()`

v1: https://patchwork.libcamera.org/patch/24144/
---
 src/apps/cam/main.cpp    | 57 ++++++++++++++++++----------------------
 src/apps/cam/meson.build |  1 +
 2 files changed, 26 insertions(+), 32 deletions(-)

--
2.50.1

Hi Barnabás,

Thank you for the patch.

On Wed, Aug 20, 2025 at 10:06:32AM +0200, Barnabás Pőcze wrote:
> Use a signalfd to detect `SIGINT` instead of registering a signal handler
> in order to remove the `CamApp` singleton. This is better than using a
> normal signal handler: a signal can arrive after the `CamApp` object has
> been destroyed, leading to a use-after-free. Modifying the `CamApp::app_`
> in the destructor is not a good alternative because that would not be async
> signal safe (unless it is made atomic).
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
> changes in v2:
> 	* move into `CamApp::init()`
> 	* use `pthread_sigmask()` instead of `sigprocmask()`
> 
> v1: https://patchwork.libcamera.org/patch/24144/
> ---
>  src/apps/cam/main.cpp    | 57 ++++++++++++++++++----------------------
>  src/apps/cam/meson.build |  1 +
>  2 files changed, 26 insertions(+), 32 deletions(-)
> 
> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp
> index a1788b7a9..91a29736f 100644
> --- a/src/apps/cam/main.cpp
> +++ b/src/apps/cam/main.cpp
> @@ -10,8 +10,10 @@
>  #include <atomic>
>  #include <iomanip>
>  #include <iostream>
> +#include <pthread.h>
>  #include <signal.h>
>  #include <string.h>
> +#include <sys/signalfd.h>
> 
>  #include <libcamera/libcamera.h>
>  #include <libcamera/property_ids.h>
> @@ -29,13 +31,10 @@ class CamApp
>  public:
>  	CamApp();
> 
> -	static CamApp *instance();
> -
>  	int init(int argc, char **argv);
>  	void cleanup();
> 
>  	int exec();
> -	void quit();
> 
>  private:
>  	void cameraAdded(std::shared_ptr<Camera> cam);
> @@ -46,32 +45,45 @@ private:
> 
>  	static std::string cameraName(const Camera *camera);
> 
> -	static CamApp *app_;
>  	OptionsParser::Options options_;
> 
> +	UniqueFD signalFd_;
> +
>  	std::unique_ptr<CameraManager> cm_;
> 
>  	std::atomic_uint loopUsers_;
>  	EventLoop loop_;
>  };
> 
> -CamApp *CamApp::app_ = nullptr;
> -
>  CamApp::CamApp()
>  	: loopUsers_(0)
>  {
> -	CamApp::app_ = this;
> -}
> -
> -CamApp *CamApp::instance()
> -{
> -	return CamApp::app_;
>  }
> 
>  int CamApp::init(int argc, char **argv)
>  {
> +	sigset_t ss;
>  	int ret;
> 
> +	sigemptyset(&ss);
> +	sigaddset(&ss, SIGINT);
> +

I would have added a comment here along the lines of

	/*
	 * Block the SIG_INT signal to ensure it gets delivered through signalfd
	 * only.
	 */

as I had to read documentation to understand why this was needed. Up to
you, in either case,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	ret = -pthread_sigmask(SIG_BLOCK, &ss, nullptr);

Can't they return a negative error number like everybody else ? :-)

> +	if (ret < 0)
> +		return ret;
> +
> +	signalFd_.reset(signalfd(-1, &ss, SFD_CLOEXEC | SFD_NONBLOCK));
> +	if (!signalFd_.isValid())
> +		return -errno;
> +
> +	loop_.addFdEvent(signalFd_.get(), EventLoop::Read, [this] {
> +		signalfd_siginfo si;
> +		std::ignore = read(signalFd_.get(), &si, sizeof(si));
> +
> +		std::cout << "Exiting" << std::endl;
> +		loop_.exit();
> +	});
> +
>  	ret = parseOptions(argc, argv);
>  	if (ret < 0)
>  		return ret;
> @@ -103,11 +115,6 @@ int CamApp::exec()
>  	return ret;
>  }
> 
> -void CamApp::quit()
> -{
> -	loop_.exit();
> -}
> -
>  int CamApp::parseOptions(int argc, char *argv[])
>  {
>  	StreamKeyValueParser streamKeyValue;
> @@ -205,7 +212,7 @@ void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
>  void CamApp::captureDone()
>  {
>  	if (--loopUsers_ == 0)
> -		EventLoop::instance()->exit(0);
> +		loop_.exit(0);
>  }
> 
>  int CamApp::run()
> @@ -345,16 +352,6 @@ std::string CamApp::cameraName(const Camera *camera)
>  	return name;
>  }
> 
> -namespace {
> -
> -void signalHandler([[maybe_unused]] int signal)
> -{
> -	std::cout << "Exiting" << std::endl;
> -	CamApp::instance()->quit();
> -}
> -
> -} /* namespace */
> -
>  int main(int argc, char **argv)
>  {
>  	CamApp app;
> @@ -364,10 +361,6 @@ int main(int argc, char **argv)
>  	if (ret)
>  		return ret == -EINTR ? 0 : EXIT_FAILURE;
> 
> -	struct sigaction sa = {};
> -	sa.sa_handler = &signalHandler;
> -	sigaction(SIGINT, &sa, nullptr);
> -
>  	if (app.exec())
>  		return EXIT_FAILURE;
> 
> diff --git a/src/apps/cam/meson.build b/src/apps/cam/meson.build
> index 2833c86e9..cd7f120f9 100644
> --- a/src/apps/cam/meson.build
> +++ b/src/apps/cam/meson.build
> @@ -56,6 +56,7 @@ cam  = executable('cam', cam_sources,
>                        libjpeg,
>                        libsdl2,
>                        libtiff,
> +                      libthreads,
>                        libyaml,
>                    ],
>                    cpp_args : cam_cpp_args,