[{"id":35534,"web_url":"https://patchwork.libcamera.org/comment/35534/","msgid":"<20250820202425.GA17759@pendragon.ideasonboard.com>","date":"2025-08-20T20:24:25","subject":"Re: [RFC PATCH v2] apps: cam: Use signalfd","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Wed, Aug 20, 2025 at 10:06:32AM +0200, Barnabás Pőcze wrote:\n> Use a signalfd to detect `SIGINT` instead of registering a signal handler\n> in order to remove the `CamApp` singleton. This is better than using a\n> normal signal handler: a signal can arrive after the `CamApp` object has\n> been destroyed, leading to a use-after-free. Modifying the `CamApp::app_`\n> in the destructor is not a good alternative because that would not be async\n> signal safe (unless it is made atomic).\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> changes in v2:\n> \t* move into `CamApp::init()`\n> \t* use `pthread_sigmask()` instead of `sigprocmask()`\n> \n> v1: https://patchwork.libcamera.org/patch/24144/\n> ---\n> src/apps/cam/main.cpp | 57 ++++++++++++++++++----------------------\n> src/apps/cam/meson.build | 1 +\n> 2 files changed, 26 insertions(+), 32 deletions(-)\n> \n> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp\n> index a1788b7a9..91a29736f 100644\n> --- a/src/apps/cam/main.cpp\n> +++ b/src/apps/cam/main.cpp\n> @@ -10,8 +10,10 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> +#include \n> \n> #include \n> #include \n> @@ -29,13 +31,10 @@ class CamApp\n> public:\n> \tCamApp();\n> \n> -\tstatic CamApp *instance();\n> -\n> \tint init(int argc, char **argv);\n> \tvoid cleanup();\n> \n> \tint exec();\n> -\tvoid quit();\n> \n> private:\n> \tvoid cameraAdded(std::shared_ptr cam);\n> @@ -46,32 +45,45 @@ private:\n> \n> \tstatic std::string cameraName(const Camera *camera);\n> \n> -\tstatic CamApp *app_;\n> \tOptionsParser::Options options_;\n> \n> +\tUniqueFD signalFd_;\n> +\n> \tstd::unique_ptr cm_;\n> \n> \tstd::atomic_uint loopUsers_;\n> \tEventLoop loop_;\n> };\n> \n> -CamApp *CamApp::app_ = nullptr;\n> -\n> CamApp::CamApp()\n> \t: loopUsers_(0)\n> {\n> -\tCamApp::app_ = this;\n> -}\n> -\n> -CamApp *CamApp::instance()\n> -{\n> -\treturn CamApp::app_;\n> }\n> \n> int CamApp::init(int argc, char **argv)\n> {\n> +\tsigset_t ss;\n> \tint ret;\n> \n> +\tsigemptyset(&ss);\n> +\tsigaddset(&ss, SIGINT);\n> +\n\nI would have added a comment here along the lines of\n\n\t/*\n\t * Block the SIG_INT signal to ensure it gets delivered through signalfd\n\t * only.\n\t */\n\nas I had to read documentation to understand why this was needed. Up to\nyou, in either case,\n\nReviewed-by: Laurent Pinchart \n\n> +\tret = -pthread_sigmask(SIG_BLOCK, &ss, nullptr);\n\nCan't they return a negative error number like everybody else ? :-)\n\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tsignalFd_.reset(signalfd(-1, &ss, SFD_CLOEXEC | SFD_NONBLOCK));\n> +\tif (!signalFd_.isValid())\n> +\t\treturn -errno;\n> +\n> +\tloop_.addFdEvent(signalFd_.get(), EventLoop::Read, [this] {\n> +\t\tsignalfd_siginfo si;\n> +\t\tstd::ignore = read(signalFd_.get(), &si, sizeof(si));\n> +\n> +\t\tstd::cout << \"Exiting\" << std::endl;\n> +\t\tloop_.exit();\n> +\t});\n> +\n> \tret = parseOptions(argc, argv);\n> \tif (ret < 0)\n> \t\treturn ret;\n> @@ -103,11 +115,6 @@ int CamApp::exec()\n> \treturn ret;\n> }\n> \n> -void CamApp::quit()\n> -{\n> -\tloop_.exit();\n> -}\n> -\n> int CamApp::parseOptions(int argc, char *argv[])\n> {\n> \tStreamKeyValueParser streamKeyValue;\n> @@ -205,7 +212,7 @@ void CamApp::cameraRemoved(std::shared_ptr cam)\n> void CamApp::captureDone()\n> {\n> \tif (--loopUsers_ == 0)\n> -\t\tEventLoop::instance()->exit(0);\n> +\t\tloop_.exit(0);\n> }\n> \n> int CamApp::run()\n> @@ -345,16 +352,6 @@ std::string CamApp::cameraName(const Camera *camera)\n> \treturn name;\n> }\n> \n> -namespace {\n> -\n> -void signalHandler([[maybe_unused]] int signal)\n> -{\n> -\tstd::cout << \"Exiting\" << std::endl;\n> -\tCamApp::instance()->quit();\n> -}\n> -\n> -} /* namespace */\n> -\n> int main(int argc, char **argv)\n> {\n> \tCamApp app;\n> @@ -364,10 +361,6 @@ int main(int argc, char **argv)\n> \tif (ret)\n> \t\treturn ret == -EINTR ? 0 : EXIT_FAILURE;\n> \n> -\tstruct sigaction sa = {};\n> -\tsa.sa_handler = &signalHandler;\n> -\tsigaction(SIGINT, &sa, nullptr);\n> -\n> \tif (app.exec())\n> \t\treturn EXIT_FAILURE;\n> \n> diff --git a/src/apps/cam/meson.build b/src/apps/cam/meson.build\n> index 2833c86e9..cd7f120f9 100644\n> --- a/src/apps/cam/meson.build\n> +++ b/src/apps/cam/meson.build\n> @@ -56,6 +56,7 @@ cam = executable('cam', cam_sources,\n> libjpeg,\n> libsdl2,\n> libtiff,\n> + libthreads,\n> libyaml,\n> ],\n> cpp_args : cam_cpp_args,","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C16FEBD87C\n\tfor ;\n\tWed, 20 Aug 2025 20:24:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A8AE692DE;\n\tWed, 20 Aug 2025 22:24:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 540586142C\n\tfor ;\n\tWed, 20 Aug 2025 22:24:49 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 68AA26AF;\n\tWed, 20 Aug 2025 22:23:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"wJ62QTer\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755721430;\n\tbh=JVIIIxYhfT98lQ0D2J+aVLHSjqTSnT3zkXoFPcyZpcI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wJ62QTerpZQWDxC28lOtD1Rcw2cvs1N119thEU8RA95CyTLjdavyDQpgpbj5v7Y62\n\toBnM3/JkuteC2HAhNmau8nWGx/V73XJ8wMOBNN5nwI80FyOByi9gcNIpah3+0NVFdG\n\tLECOMJqyZLSAKe2hpB0DZEjCxrM7PGHLn/CrdAhU=","Date":"Wed, 20 Aug 2025 23:24:25 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [RFC PATCH v2] apps: cam: Use signalfd","Message-ID":"<20250820202425.GA17759@pendragon.ideasonboard.com>","References":"<20250820080632.106505-1-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250820080632.106505-1-barnabas.pocze@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36070,"web_url":"https://patchwork.libcamera.org/comment/36070/","msgid":"<175939439198.756374.5019883976208448529@ping.linuxembedded.co.uk>","date":"2025-10-02T08:39:51","subject":"Re: [RFC PATCH v2] apps: cam: Use signalfd","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-08-20 09:06:32)\n> Use a signalfd to detect `SIGINT` instead of registering a signal handler\n> in order to remove the `CamApp` singleton. This is better than using a\n> normal signal handler: a signal can arrive after the `CamApp` object has\n> been destroyed, leading to a use-after-free. Modifying the `CamApp::app_`\n> in the destructor is not a good alternative because that would not be async\n> signal safe (unless it is made atomic).\n> \n> Signed-off-by: Barnabás Pőcze \n\nReviewed-by: Kieran Bingham \n\n> ---\n> changes in v2:\n> * move into `CamApp::init()`\n> * use `pthread_sigmask()` instead of `sigprocmask()`\n> \n> v1: https://patchwork.libcamera.org/patch/24144/\n> ---\n> src/apps/cam/main.cpp | 57 ++++++++++++++++++----------------------\n> src/apps/cam/meson.build | 1 +\n> 2 files changed, 26 insertions(+), 32 deletions(-)\n> \n> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp\n> index a1788b7a9..91a29736f 100644\n> --- a/src/apps/cam/main.cpp\n> +++ b/src/apps/cam/main.cpp\n> @@ -10,8 +10,10 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> +#include \n> \n> #include \n> #include \n> @@ -29,13 +31,10 @@ class CamApp\n> public:\n> CamApp();\n> \n> - static CamApp *instance();\n> -\n> int init(int argc, char **argv);\n> void cleanup();\n> \n> int exec();\n> - void quit();\n> \n> private:\n> void cameraAdded(std::shared_ptr cam);\n> @@ -46,32 +45,45 @@ private:\n> \n> static std::string cameraName(const Camera *camera);\n> \n> - static CamApp *app_;\n> OptionsParser::Options options_;\n> \n> + UniqueFD signalFd_;\n> +\n> std::unique_ptr cm_;\n> \n> std::atomic_uint loopUsers_;\n> EventLoop loop_;\n> };\n> \n> -CamApp *CamApp::app_ = nullptr;\n> -\n> CamApp::CamApp()\n> : loopUsers_(0)\n> {\n> - CamApp::app_ = this;\n> -}\n> -\n> -CamApp *CamApp::instance()\n> -{\n> - return CamApp::app_;\n> }\n> \n> int CamApp::init(int argc, char **argv)\n> {\n> + sigset_t ss;\n> int ret;\n> \n> + sigemptyset(&ss);\n> + sigaddset(&ss, SIGINT);\n> +\n> + ret = -pthread_sigmask(SIG_BLOCK, &ss, nullptr);\n> + if (ret < 0)\n> + return ret;\n> +\n> + signalFd_.reset(signalfd(-1, &ss, SFD_CLOEXEC | SFD_NONBLOCK));\n> + if (!signalFd_.isValid())\n> + return -errno;\n> +\n> + loop_.addFdEvent(signalFd_.get(), EventLoop::Read, [this] {\n> + signalfd_siginfo si;\n> + std::ignore = read(signalFd_.get(), &si, sizeof(si));\n> +\n> + std::cout << \"Exiting\" << std::endl;\n> + loop_.exit();\n> + });\n> +\n> ret = parseOptions(argc, argv);\n> if (ret < 0)\n> return ret;\n> @@ -103,11 +115,6 @@ int CamApp::exec()\n> return ret;\n> }\n> \n> -void CamApp::quit()\n> -{\n> - loop_.exit();\n> -}\n> -\n> int CamApp::parseOptions(int argc, char *argv[])\n> {\n> StreamKeyValueParser streamKeyValue;\n> @@ -205,7 +212,7 @@ void CamApp::cameraRemoved(std::shared_ptr cam)\n> void CamApp::captureDone()\n> {\n> if (--loopUsers_ == 0)\n> - EventLoop::instance()->exit(0);\n> + loop_.exit(0);\n> }\n> \n> int CamApp::run()\n> @@ -345,16 +352,6 @@ std::string CamApp::cameraName(const Camera *camera)\n> return name;\n> }\n> \n> -namespace {\n> -\n> -void signalHandler([[maybe_unused]] int signal)\n> -{\n> - std::cout << \"Exiting\" << std::endl;\n> - CamApp::instance()->quit();\n> -}\n> -\n> -} /* namespace */\n> -\n> int main(int argc, char **argv)\n> {\n> CamApp app;\n> @@ -364,10 +361,6 @@ int main(int argc, char **argv)\n> if (ret)\n> return ret == -EINTR ? 0 : EXIT_FAILURE;\n> \n> - struct sigaction sa = {};\n> - sa.sa_handler = &signalHandler;\n> - sigaction(SIGINT, &sa, nullptr);\n> -\n> if (app.exec())\n> return EXIT_FAILURE;\n> \n> diff --git a/src/apps/cam/meson.build b/src/apps/cam/meson.build\n> index 2833c86e9..cd7f120f9 100644\n> --- a/src/apps/cam/meson.build\n> +++ b/src/apps/cam/meson.build\n> @@ -56,6 +56,7 @@ cam = executable('cam', cam_sources,\n> libjpeg,\n> libsdl2,\n> libtiff,\n> + libthreads,\n> libyaml,\n> ],\n> cpp_args : cam_cpp_args,\n> --\n> 2.50.1","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 05FA7C328C\n\tfor ;\n\tThu, 2 Oct 2025 08:39:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 99E926B5AA;\n\tThu, 2 Oct 2025 10:39:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4ED9D613AB\n\tfor ;\n\tThu, 2 Oct 2025 10:39:55 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4B1C7BCA;\n\tThu, 2 Oct 2025 10:38:25 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"D6JuSLU/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1759394305;\n\tbh=0azxmaY+Y5ijyyT11LloIWbBMwE9hZK31CRO3r+NzmM=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=D6JuSLU/nNWZ/5bACEK1x/F6ODt54Bmckb4ovWITNM6sBvyVyNecMpa0nN9t7f5ap\n\tNi5jbpa7J/0r021wnVYSHWycZbCTWar+YUzbTjreTOEmgG8tntAFMzqr6b47n+HLuB\n\tamePYBg57o+zq1qNAnkENw+p6qSNoQgQc4vEe+BE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250820080632.106505-1-barnabas.pocze@ideasonboard.com>","References":"<20250820080632.106505-1-barnabas.pocze@ideasonboard.com>","Subject":"Re: [RFC PATCH v2] apps: cam: Use signalfd","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tPaul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 02 Oct 2025 09:39:51 +0100","Message-ID":"<175939439198.756374.5019883976208448529@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]