[{"id":18240,"web_url":"https://patchwork.libcamera.org/comment/18240/","msgid":"","date":"2021-07-20T16:59:42","subject":"Re: [libcamera-devel] [RFC PATCH] ipa: IPADataSerializer: Fix\n\tFileDescriptor deserialization","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Tue, Jul 20, 2021 at 07:28:53PM +0900, Paul Elder wrote:\n> Previously deserializing a FileDescriptor would use the copy\n> constructor, causing an fd leak. Fix this by copying the fd integer to\n> force the FileDescriptor to use the move constructor.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Umang, could you please check if this fixes the issue?\n> \n> I'll remove the error message; it's just there for debugging (or we can\n> keep it to protect against unforseen errors?)\n> ---\n> src/libcamera/ipa_data_serializer.cpp | 10 +++++++++-\n> 1 file changed, 9 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp\n> index fb941e6b..2a527d5d 100644\n> --- a/src/libcamera/ipa_data_serializer.cpp\n> +++ b/src/libcamera/ipa_data_serializer.cpp\n> @@ -8,6 +8,7 @@\n> #include \"libcamera/internal/ipa_data_serializer.h\"\n> \n> #include \n> +#include \n> \n> /**\n> * \\file ipa_data_serializer.h\n> @@ -547,7 +548,14 @@ FileDescriptor IPADataSerializer::deserialize(std::vector \n> \tASSERT(!(valid && std::distance(fdsBegin, fdsEnd) < 1));\n> \n> -\treturn valid ? FileDescriptor(*fdsBegin) : FileDescriptor();\n> +\tint tmpfd = valid ? *fdsBegin : -1;\n> +\tFileDescriptor fd = FileDescriptor(tmpfd);\n\nDon't you need a std::move(tmpfd) here to use the move constructor ?\n\nTo be honest, I'm still feeling a bit uneasy about the large distance\nbetween the fd creation (when we receive the data from the unix socket)\nand the wrapping in a FileDescriptor. There's nothing that protects\nagainst leaks between those two points. At the very least I think we\nshould turn the const iterator to a non-const iterator, and walk the\nvector of file descriptors after we're done deserializing to ensure that\nall entries are equal to -1, as a defensive measure, but then I'm not\nsure why we shouldn't go all the way and use FileDescriptor in the IPC\nmessage. The more we can use the language features to ensure that bugs\ncan't happen, the better.\n\n> +\tif (valid && tmpfd != -1) {\n> +\t\tLOG(IPADataSerializer, Error) << \"We probably leaked an fd\";\n> +\t\tclose(tmpfd);\n> +\t}\n> +\n> +\treturn fd;\n> }\n> \n> template<>","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 1655BC0109\n\tfor ;\n\tTue, 20 Jul 2021 16:59:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8B7C86852A;\n\tTue, 20 Jul 2021 18:59:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E377468521\n\tfor ;\n\tTue, 20 Jul 2021 18:59:47 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6015751D;\n\tTue, 20 Jul 2021 18:59:47 +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=\"s+WR0yuj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626800387;\n\tbh=8JlE00lni4lyiSAPNiz9wvORaUx7IpzybAYeEDTnj9Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=s+WR0yujC65sXjkL5oPmE1vPK7412l306kYyHJK19TzAK3o2ru2Cyt/3LQ7wHqkRf\n\tGqC6fGMHmkzavDIyAPCV/1zFCERjK622c6dXwBK+HlPuTRz9r4dlpAmM7QlFwMf3th\n\to93xHwG5C9YX5GuyrG4mCWjfRm5nJv3q16lS25w4=","Date":"Tue, 20 Jul 2021 19:59:42 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210720102853.28541-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210720102853.28541-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH] ipa: IPADataSerializer: Fix\n\tFileDescriptor deserialization","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]