[v2] utils: codegen: ipc: Check `ipc_` instead of `isolate_`
diff mbox series

Message ID 20250722145334.1777305-1-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • [v2] utils: codegen: ipc: Check `ipc_` instead of `isolate_`
Related show

Commit Message

Barnabás Pőcze July 22, 2025, 2:53 p.m. UTC 
Only try to send the "Exit" message if the `ipc_` pointer is non-null. If
the constructor fails, then `ipc_` might remain nullptr, which would lead
to a nullptr dereference in the destructor.

This change also modifies the constructor so that only a valid `IPCPipeUnixSocket`
object will be saved into the `ipc_` member, which avoids error messages when
`IPCPipeUnixSocket::sendAsync()` is called in the inappropriate state in the
destructor.

Bug: https://bugs.libcamera.org/show_bug.cgi?id=276
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
changes in v2:
  * modify constructor not to save "invalid" IPCPipeUnixSocket into member

v1: https://patchwork.libcamera.org/patch/23894/
---
 .../libcamera_templates/module_ipa_proxy.cpp.tmpl     | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

--
2.50.1

Comments

Paul Elder July 24, 2025, 8:34 a.m. UTC | #1 
Hi Barnabás,

Quoting Barnabás Pőcze (2025-07-22 23:53:34)
> Only try to send the "Exit" message if the `ipc_` pointer is non-null. If
> the constructor fails, then `ipc_` might remain nullptr, which would lead
> to a nullptr dereference in the destructor.
> 
> This change also modifies the constructor so that only a valid `IPCPipeUnixSocket`
> object will be saved into the `ipc_` member, which avoids error messages when
> `IPCPipeUnixSocket::sendAsync()` is called in the inappropriate state in the
> destructor.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=276
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
> changes in v2:
>   * modify constructor not to save "invalid" IPCPipeUnixSocket into member
> 
> v1: https://patchwork.libcamera.org/patch/23894/
> ---
>  .../libcamera_templates/module_ipa_proxy.cpp.tmpl     | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> index 0f87e7976..18b4ab5e5 100644
> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> @@ -61,15 +61,16 @@ namespace {{ns}} {
>                         return;
>                 }
> 
> -               ipc_ = std::make_unique<IPCPipeUnixSocket>(ipam->path().c_str(),
> -                                                          proxyWorkerPath.c_str());
> -               if (!ipc_->isConnected()) {
> +               auto ipc = std::make_unique<IPCPipeUnixSocket>(ipam->path().c_str(),
> +                                                              proxyWorkerPath.c_str());
> +               if (!ipc->isConnected()) {
>                         LOG(IPAProxy, Error) << "Failed to create IPCPipe";
>                         return;
>                 }
> 
> -               ipc_->recv.connect(this, &{{proxy_name}}::recvMessage);
> +               ipc->recv.connect(this, &{{proxy_name}}::recvMessage);
> 
> +               ipc_ = std::move(ipc);

Oh, nice.

>                 valid_ = true;
>                 return;
>         }
> @@ -96,7 +97,7 @@ namespace {{ns}} {
> 
>  {{proxy_name}}::~{{proxy_name}}()
>  {
> -       if (isolate_) {
> +       if (ipc_) {

Oh ok I see it now.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

>                 IPCMessage::Header header =
>                         { static_cast<uint32_t>({{cmd_enum_name}}::Exit), seq_++ };
>                 IPCMessage msg(header);
> --
> 2.50.1
Kieran Bingham July 24, 2025, 8:58 a.m. UTC | #2 
Quoting Barnabás Pőcze (2025-07-22 15:53:34)
> Only try to send the "Exit" message if the `ipc_` pointer is non-null. If
> the constructor fails, then `ipc_` might remain nullptr, which would lead
> to a nullptr dereference in the destructor.
> 
> This change also modifies the constructor so that only a valid `IPCPipeUnixSocket`
> object will be saved into the `ipc_` member, which avoids error messages when
> `IPCPipeUnixSocket::sendAsync()` is called in the inappropriate state in the
> destructor.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=276
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
> changes in v2:
>   * modify constructor not to save "invalid" IPCPipeUnixSocket into member
> 
> v1: https://patchwork.libcamera.org/patch/23894/
> ---
>  .../libcamera_templates/module_ipa_proxy.cpp.tmpl     | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> index 0f87e7976..18b4ab5e5 100644
> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> @@ -61,15 +61,16 @@ namespace {{ns}} {
>                         return;
>                 }
> 
> -               ipc_ = std::make_unique<IPCPipeUnixSocket>(ipam->path().c_str(),
> -                                                          proxyWorkerPath.c_str());
> -               if (!ipc_->isConnected()) {
> +               auto ipc = std::make_unique<IPCPipeUnixSocket>(ipam->path().c_str(),
> +                                                              proxyWorkerPath.c_str());
> +               if (!ipc->isConnected()) {
>                         LOG(IPAProxy, Error) << "Failed to create IPCPipe";
>                         return;
>                 }
> 
> -               ipc_->recv.connect(this, &{{proxy_name}}::recvMessage);
> +               ipc->recv.connect(this, &{{proxy_name}}::recvMessage);
> 
> +               ipc_ = std::move(ipc);
>                 valid_ = true;
>                 return;
>         }
> @@ -96,7 +97,7 @@ namespace {{ns}} {
> 
>  {{proxy_name}}::~{{proxy_name}}()
>  {
> -       if (isolate_) {
> +       if (ipc_) {
>                 IPCMessage::Header header =
>                         { static_cast<uint32_t>({{cmd_enum_name}}::Exit), seq_++ };
>                 IPCMessage msg(header);
> --
> 2.50.1

Patch
diff mbox series 

diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
index 0f87e7976..18b4ab5e5 100644
--- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
+++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
@@ -61,15 +61,16 @@  namespace {{ns}} {
 			return;
 		}

-		ipc_ = std::make_unique<IPCPipeUnixSocket>(ipam->path().c_str(),
-							   proxyWorkerPath.c_str());
-		if (!ipc_->isConnected()) {
+		auto ipc = std::make_unique<IPCPipeUnixSocket>(ipam->path().c_str(),
+							       proxyWorkerPath.c_str());
+		if (!ipc->isConnected()) {
 			LOG(IPAProxy, Error) << "Failed to create IPCPipe";
 			return;
 		}

-		ipc_->recv.connect(this, &{{proxy_name}}::recvMessage);
+		ipc->recv.connect(this, &{{proxy_name}}::recvMessage);

+		ipc_ = std::move(ipc);
 		valid_ = true;
 		return;
 	}
@@ -96,7 +97,7 @@  namespace {{ns}} {

 {{proxy_name}}::~{{proxy_name}}()
 {
-	if (isolate_) {
+	if (ipc_) {
 		IPCMessage::Header header =
 			{ static_cast<uint32_t>({{cmd_enum_name}}::Exit), seq_++ };
 		IPCMessage msg(header);