[{"id":3317,"web_url":"https://patchwork.libcamera.org/comment/3317/","msgid":"<20200104002237.GH4847@pendragon.ideasonboard.com>","date":"2020-01-04T00:22:37","subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: utils: Add strlcpy","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 Fri, Jan 03, 2020 at 07:14:09PM -0500, Paul Elder wrote:\n> strlcpy is available in libbsd, bionic, musl, and ulibc, but not in glibc.\n> Instead of checking for strlcpy availability and modifying dependencies,\n> implement it in utils, as a wrapper around strncpy.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> New in v6\n> ---\n> src/libcamera/include/utils.h | 3 +++\n> src/libcamera/utils.cpp | 19 +++++++++++++++++++\n> 2 files changed, 22 insertions(+)\n> \n> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h\n> index a80f7d09..badc7753 100644\n> --- a/src/libcamera/include/utils.h\n> +++ b/src/libcamera/include/utils.h\n> @@ -12,6 +12,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> \n> #define ARRAY_SIZE(a)\t(sizeof(a) / sizeof(a[0]))\n> @@ -112,6 +113,8 @@ inline _hex hex(uint64_t value, unsigned int width)\n> }\n> #endif\n> \n> +size_t strlcpy(char *dst, const char *src, size_t size);\n> +\n> } /* namespace utils */\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> index d632f6e6..2d6e082e 100644\n> --- a/src/libcamera/utils.cpp\n> +++ b/src/libcamera/utils.cpp\n> @@ -182,6 +182,25 @@ operator<<(std::basic_ostream> &stream, const _hex\n> * otherwise. The \\a os stream configuration is not modified.\n> */\n> \n> +/**\n> + * \\brief Copy a string with a size limit\n> + * \\param[in] dst The destination string\n> + * \\param[in] src The source string\n> + * \\param[in] size The size of the destination string\n> + *\n> + * strlcpy is available in libbsd, bionic, musl, and ulibc, but not in glibc.\n> + * Instead of checking for strlcpy availability and modifying dependencies,\n> + * it is implemented here as a wrapper around strncpy.\n\nShouldn't you instead document what the function does ?\n\n * This function copy the null-terminated string \\a src to \\a dst with a limit\n * of \\a size - 1 characters, and null-terminates the result if \\a size is\n * larger than 0. If \\a src is larger than \\a size - 1, \\a dst is truncated.\n\n> + *\n> + * \\return The size of \\a src\n> + */\n> +size_t strlcpy(char *dst, const char *src, size_t size)\n> +{\n\n\tif (size) {\n\n> +\tstrncpy(dst, src, size);\n> +\tdst[size - 1] = '\\0';\n\n\t}\n\njust to be safe ?\n\nReviewed-by: Laurent Pinchart \n\n> +\treturn strlen(src);\n> +}\n> +\n> } /* namespace utils */\n> \n> } /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AC463605F8\n\tfor ;\n\tSat, 4 Jan 2020 01:22:48 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 329EF30F;\n\tSat, 4 Jan 2020 01:22:48 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578097368;\n\tbh=B3k+wCDQIiKgHPPfC/4K4zvKSU/APf57L6GtsW7JAj8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DN7hNa7PU7x1FaSdgmaZFicmnscggltbLkmdJ8VhE1TSBw0jyAcKd1sDmgQGu0n9+\n\tzTjLpdQMFVIJbhY4YpEZUte1Oai3dkQyjoXJflgSpoiP3kOBDMDNFK/qb1DmO1vT5D\n\tlpDlToantjBQHDlhgetTxUk2WUcctjBDTQs52dy8=","Date":"Sat, 4 Jan 2020 02:22:37 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200104002237.GH4847@pendragon.ideasonboard.com>","References":"<20200104001414.12755-1-paul.elder@ideasonboard.com>\n\t<20200104001414.12755-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200104001414.12755-2-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: utils: Add strlcpy","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","X-List-Received-Date":"Sat, 04 Jan 2020 00:22:48 -0000"}},{"id":3321,"web_url":"https://patchwork.libcamera.org/comment/3321/","msgid":"","date":"2020-01-06T09:38:10","subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: utils: Add strlcpy","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Paul, Laurent,\n\nOnly discussion here, as I see this patch is merged (which is where I\nsaw it and it caught my attention)\n\nOn 04/01/2020 00:22, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Fri, Jan 03, 2020 at 07:14:09PM -0500, Paul Elder wrote:\n>> strlcpy is available in libbsd, bionic, musl, and ulibc, but not in glibc.\n>> Instead of checking for strlcpy availability and modifying dependencies,\n>> implement it in utils, as a wrapper around strncpy.\n>>\n\nOh wow, I just had a fun read up on this topic [0], [1]. People get\nreally picky about string handling :D\n\n[1] highlights the following:\n\n> The primary complaint about strlcpy() is that it gives no indication\n> of whether the copied string was truncated or not. So careless code\n> using strlcpy() risks replacing a buffer-overrun error with a\n> different problem resulting from corrupted strings. In the minds of\n> many developers, strlcpy() just replaces one issue with another and,\n> thus, does not solve the problem of safe string handling in C\n> programs. They believe it should not be used, and, since it should\n> not be used, it also should not be provided by the library.\nPersonally, I disagree with the \"Should not be provided by the library\"\non the basis which is also discussed that now there are N variable\nimplementations all with their own bugs.\n\nThat said, I don't mind adding this in here, as long as we make sure\nwe're careful with it, thus see below:\n\n\n[0] The ups and downs of strlcpy() (Michael Kerrisk)\n https://lwn.net/Articles/507319/\n\n[1] Adding strlcpy() to glibc (Jonathon Corbet)\n https://lwn.net/Articles/612244/\n\n\n>> Signed-off-by: Paul Elder \n>>\n>> ---\n>> New in v6\n>> ---\n>> src/libcamera/include/utils.h | 3 +++\n>> src/libcamera/utils.cpp | 19 +++++++++++++++++++\n>> 2 files changed, 22 insertions(+)\n>>\n>> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h\n>> index a80f7d09..badc7753 100644\n>> --- a/src/libcamera/include/utils.h\n>> +++ b/src/libcamera/include/utils.h\n>> @@ -12,6 +12,7 @@\n>> #include \n>> #include \n>> #include \n>> +#include \n>> #include \n>> \n>> #define ARRAY_SIZE(a)\t(sizeof(a) / sizeof(a[0]))\n>> @@ -112,6 +113,8 @@ inline _hex hex(uint64_t value, unsigned int width)\n>> }\n>> #endif\n>> \n>> +size_t strlcpy(char *dst, const char *src, size_t size);\n>> +\n>> } /* namespace utils */\n>> \n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n>> index d632f6e6..2d6e082e 100644\n>> --- a/src/libcamera/utils.cpp\n>> +++ b/src/libcamera/utils.cpp\n>> @@ -182,6 +182,25 @@ operator<<(std::basic_ostream> &stream, const _hex\n>> * otherwise. The \\a os stream configuration is not modified.\n>> */\n>> \n>> +/**\n>> + * \\brief Copy a string with a size limit\n>> + * \\param[in] dst The destination string\n>> + * \\param[in] src The source string\n>> + * \\param[in] size The size of the destination string\n>> + *\n>> + * strlcpy is available in libbsd, bionic, musl, and ulibc, but not in glibc.\n>> + * Instead of checking for strlcpy availability and modifying dependencies,\n>> + * it is implemented here as a wrapper around strncpy.\n> \n> Shouldn't you instead document what the function does ?\n> \n> * This function copy the null-terminated string \\a src to \\a dst with a limit> * of \\a size - 1 characters, and null-terminates the result if \\a size is\n> * larger than 0. If \\a src is larger than \\a size - 1, \\a dst is truncated.\n> \n\n\"Users should check the return value of this call against the size of\nthe destination buffer to determine if truncation occurs.\" ?\n\n>> + *\n>> + * \\return The size of \\a src\n>> + */\n>> +size_t strlcpy(char *dst, const char *src, size_t size)\n>> +{\n> \n> \tif (size) {\n> \n>> +\tstrncpy(dst, src, size);\n>> +\tdst[size - 1] = '\\0';\n> \n> \t}\n> \n> just to be safe ?\n> \n> Reviewed-by: Laurent Pinchart \n> \n>> +\treturn strlen(src);\n\n\nSo the main thing that caught my eye here was the strlen(src) (rather\nthan dst) here, which is because strlcpy returns the length of the\nsource which must be checked against for truncation.\n\n\nShould we bring in some compiler attributes to ensure the return value\nis always checked?\n\nhttps://elixir.bootlin.com/linux/v5.5-rc5/source/include/linux/compiler_types.h#L108\n?\n\n\nSaying that, though - I see that the only uses of this call (so far) do\nnot check the return value or validate against truncation. Perhaps we're\nhappy with that, but we do so silently.\n\nOf course in those instances though - we ought to know the length of the\nsource anyway as it is defined by V4L2 I believe.\n\n--\nKieran\n\n\n\n\n>> +}\n>> +\n>> } /* namespace utils */\n>> \n>> } /* namespace libcamera */\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 02F2060461\n\tfor ;\n\tMon, 6 Jan 2020 10:38:13 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 17A6D312;\n\tMon, 6 Jan 2020 10:38:13 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578303493;\n\tbh=GSMK0Ld0RSq7PaCPyCyG2LPcwV6A43rdm6li0nuWPpM=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=LiukzqvEFSmJdLSClNpVfK2G/41MekGsQM6b9WRuK1+wylvSr6dvffSt3pCUfWqN/\n\tkg3/8kru4Lw91Z6qcph/mHO1WKfHVjUc0iKmvQ6u79rVIvw1/V3N0Kh08pLLBx3CdC\n\tn+QdqqWPvN3Q9fMqEKlNVp+dIDnhTFRRFUw5GUPA=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ,\n\tPaul Elder ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20200104001414.12755-1-paul.elder@ideasonboard.com>\n\t<20200104001414.12755-2-paul.elder@ideasonboard.com>\n\t<20200104002237.GH4847@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Mon, 6 Jan 2020 09:38:10 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.0","MIME-Version":"1.0","In-Reply-To":"<20200104002237.GH4847@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: utils: Add strlcpy","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","X-List-Received-Date":"Mon, 06 Jan 2020 09:38:14 -0000"}},{"id":3328,"web_url":"https://patchwork.libcamera.org/comment/3328/","msgid":"<20200106170706.GC13373@pendragon.ideasonboard.com>","date":"2020-01-06T17:07:06","subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: utils: Add strlcpy","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Jan 06, 2020 at 09:38:10AM +0000, Kieran Bingham wrote:\n> Hi Paul, Laurent,\n> \n> Only discussion here, as I see this patch is merged (which is where I\n> saw it and it caught my attention)\n> \n> On 04/01/2020 00:22, Laurent Pinchart wrote:\n> > Hi Paul,\n> > \n> > Thank you for the patch.\n> > \n> > On Fri, Jan 03, 2020 at 07:14:09PM -0500, Paul Elder wrote:\n> >> strlcpy is available in libbsd, bionic, musl, and ulibc, but not in glibc.\n> >> Instead of checking for strlcpy availability and modifying dependencies,\n> >> implement it in utils, as a wrapper around strncpy.\n> \n> Oh wow, I just had a fun read up on this topic [0], [1]. People get\n> really picky about string handling :D\n> \n> [1] highlights the following:\n> \n> > The primary complaint about strlcpy() is that it gives no indication\n> > of whether the copied string was truncated or not. So careless code\n> > using strlcpy() risks replacing a buffer-overrun error with a\n> > different problem resulting from corrupted strings. In the minds of\n> > many developers, strlcpy() just replaces one issue with another and,\n> > thus, does not solve the problem of safe string handling in C\n> > programs. They believe it should not be used, and, since it should\n> > not be used, it also should not be provided by the library.\n> \n> Personally, I disagree with the \"Should not be provided by the library\"\n> on the basis which is also discussed that now there are N variable\n> implementations all with their own bugs.\n> \n> That said, I don't mind adding this in here, as long as we make sure\n> we're careful with it, thus see below:\n> \n> [0] The ups and downs of strlcpy() (Michael Kerrisk)\n> https://lwn.net/Articles/507319/\n> \n> [1] Adding strlcpy() to glibc (Jonathon Corbet)\n> https://lwn.net/Articles/612244/\n> \n> >> Signed-off-by: Paul Elder \n> >>\n> >> ---\n> >> New in v6\n> >> ---\n> >> src/libcamera/include/utils.h | 3 +++\n> >> src/libcamera/utils.cpp | 19 +++++++++++++++++++\n> >> 2 files changed, 22 insertions(+)\n> >>\n> >> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h\n> >> index a80f7d09..badc7753 100644\n> >> --- a/src/libcamera/include/utils.h\n> >> +++ b/src/libcamera/include/utils.h\n> >> @@ -12,6 +12,7 @@\n> >> #include \n> >> #include \n> >> #include \n> >> +#include \n> >> #include \n> >> \n> >> #define ARRAY_SIZE(a)\t(sizeof(a) / sizeof(a[0]))\n> >> @@ -112,6 +113,8 @@ inline _hex hex(uint64_t value, unsigned int width)\n> >> }\n> >> #endif\n> >> \n> >> +size_t strlcpy(char *dst, const char *src, size_t size);\n> >> +\n> >> } /* namespace utils */\n> >> \n> >> } /* namespace libcamera */\n> >> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> >> index d632f6e6..2d6e082e 100644\n> >> --- a/src/libcamera/utils.cpp\n> >> +++ b/src/libcamera/utils.cpp\n> >> @@ -182,6 +182,25 @@ operator<<(std::basic_ostream> &stream, const _hex\n> >> * otherwise. The \\a os stream configuration is not modified.\n> >> */\n> >> \n> >> +/**\n> >> + * \\brief Copy a string with a size limit\n> >> + * \\param[in] dst The destination string\n> >> + * \\param[in] src The source string\n> >> + * \\param[in] size The size of the destination string\n> >> + *\n> >> + * strlcpy is available in libbsd, bionic, musl, and ulibc, but not in glibc.\n> >> + * Instead of checking for strlcpy availability and modifying dependencies,\n> >> + * it is implemented here as a wrapper around strncpy.\n> > \n> > Shouldn't you instead document what the function does ?\n> > \n> > * This function copy the null-terminated string \\a src to \\a dst with a limit> * of \\a size - 1 characters, and null-terminates the result if \\a size is\n> > * larger than 0. If \\a src is larger than \\a size - 1, \\a dst is truncated.\n> > \n> \n> \"Users should check the return value of this call against the size of\n> the destination buffer to determine if truncation occurs.\" ?\n> \n> >> + *\n> >> + * \\return The size of \\a src\n> >> + */\n> >> +size_t strlcpy(char *dst, const char *src, size_t size)\n> >> +{\n> > \n> > \tif (size) {\n> > \n> >> +\tstrncpy(dst, src, size);\n> >> +\tdst[size - 1] = '\\0';\n> > \n> > \t}\n> > \n> > just to be safe ?\n> > \n> > Reviewed-by: Laurent Pinchart \n> > \n> >> +\treturn strlen(src);\n> \n> So the main thing that caught my eye here was the strlen(src) (rather\n> than dst) here, which is because strlcpy returns the length of the\n> source which must be checked against for truncation.\n> \n> Should we bring in some compiler attributes to ensure the return value\n> is always checked?\n> \n> https://elixir.bootlin.com/linux/v5.5-rc5/source/include/linux/compiler_types.h#L108\n> ?\n> \n> Saying that, though - I see that the only uses of this call (so far) do\n> not check the return value or validate against truncation. Perhaps we're\n> happy with that, but we do so silently.\n> \n> Of course in those instances though - we ought to know the length of the\n> source anyway as it is defined by V4L2 I believe.\n\nIn this specific case the idea is to indeed truncate. We silently ignore\ntruncation because there's nothing we can do about it, the length of the\nbuffers is defined by the V4L2 APÏ, and they're small enough that\ntruncation happens quite often in existing kernel drivers. This is a bad\nkernel API, there's not much we can do about it :-(\n\n> >> +}\n> >> +\n> >> } /* namespace utils */\n> >> \n> >> } /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2BA3C60612\n\tfor ;\n\tMon, 6 Jan 2020 18:07:17 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 95C0D52F;\n\tMon, 6 Jan 2020 18:07:16 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578330436;\n\tbh=kOpYFC1KfYKj42r5BrqMb/SdD+RMMbJG+fyDLyUaJb8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Plb45avt09CFguJHiFxjKxNF34w9D5sPITYZZPHemP2z5RzGGJGBAenKnbc59uAS+\n\tuAuTdhI1Hah19Oo+84BKA3FJ+l4MblqxpnuXBfvdg92U/BlULGBUvAnGkxezffuTTp\n\tfKgGMtQz6w1srWvXvfzU4GWgz4vaew9zrEsfllLc=","Date":"Mon, 6 Jan 2020 19:07:06 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200106170706.GC13373@pendragon.ideasonboard.com>","References":"<20200104001414.12755-1-paul.elder@ideasonboard.com>\n\t<20200104001414.12755-2-paul.elder@ideasonboard.com>\n\t<20200104002237.GH4847@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: utils: Add strlcpy","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","X-List-Received-Date":"Mon, 06 Jan 2020 17:07:17 -0000"}},{"id":3329,"web_url":"https://patchwork.libcamera.org/comment/3329/","msgid":"<4ff9787a-4269-1213-498a-4071bdb3870e@ideasonboard.com>","date":"2020-01-06T17:10:18","subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: utils: Add strlcpy","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 06/01/2020 17:07, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Mon, Jan 06, 2020 at 09:38:10AM +0000, Kieran Bingham wrote:\n>> Hi Paul, Laurent,\n>>\n>> Only discussion here, as I see this patch is merged (which is where I\n>> saw it and it caught my attention)\n>>\n>> On 04/01/2020 00:22, Laurent Pinchart wrote:\n>>> Hi Paul,\n>>>\n>>> Thank you for the patch.\n>>>\n>>> On Fri, Jan 03, 2020 at 07:14:09PM -0500, Paul Elder wrote:\n>>>> strlcpy is available in libbsd, bionic, musl, and ulibc, but not in glibc.\n>>>> Instead of checking for strlcpy availability and modifying dependencies,\n>>>> implement it in utils, as a wrapper around strncpy.\n>>\n>> Oh wow, I just had a fun read up on this topic [0], [1]. People get\n>> really picky about string handling :D\n>>\n>> [1] highlights the following:\n>>\n>>> The primary complaint about strlcpy() is that it gives no indication\n>>> of whether the copied string was truncated or not. So careless code\n>>> using strlcpy() risks replacing a buffer-overrun error with a\n>>> different problem resulting from corrupted strings. In the minds of\n>>> many developers, strlcpy() just replaces one issue with another and,\n>>> thus, does not solve the problem of safe string handling in C\n>>> programs. They believe it should not be used, and, since it should\n>>> not be used, it also should not be provided by the library.\n>>\n>> Personally, I disagree with the \"Should not be provided by the library\"\n>> on the basis which is also discussed that now there are N variable\n>> implementations all with their own bugs.\n>>\n>> That said, I don't mind adding this in here, as long as we make sure\n>> we're careful with it, thus see below:\n>>\n>> [0] The ups and downs of strlcpy() (Michael Kerrisk)\n>> https://lwn.net/Articles/507319/\n>>\n>> [1] Adding strlcpy() to glibc (Jonathon Corbet)\n>> https://lwn.net/Articles/612244/\n>>\n>>>> Signed-off-by: Paul Elder \n>>>>\n>>>> ---\n>>>> New in v6\n>>>> ---\n>>>> src/libcamera/include/utils.h | 3 +++\n>>>> src/libcamera/utils.cpp | 19 +++++++++++++++++++\n>>>> 2 files changed, 22 insertions(+)\n>>>>\n>>>> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h\n>>>> index a80f7d09..badc7753 100644\n>>>> --- a/src/libcamera/include/utils.h\n>>>> +++ b/src/libcamera/include/utils.h\n>>>> @@ -12,6 +12,7 @@\n>>>> #include \n>>>> #include \n>>>> #include \n>>>> +#include \n>>>> #include \n>>>> \n>>>> #define ARRAY_SIZE(a)\t(sizeof(a) / sizeof(a[0]))\n>>>> @@ -112,6 +113,8 @@ inline _hex hex(uint64_t value, unsigned int width)\n>>>> }\n>>>> #endif\n>>>> \n>>>> +size_t strlcpy(char *dst, const char *src, size_t size);\n>>>> +\n>>>> } /* namespace utils */\n>>>> \n>>>> } /* namespace libcamera */\n>>>> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n>>>> index d632f6e6..2d6e082e 100644\n>>>> --- a/src/libcamera/utils.cpp\n>>>> +++ b/src/libcamera/utils.cpp\n>>>> @@ -182,6 +182,25 @@ operator<<(std::basic_ostream> &stream, const _hex\n>>>> * otherwise. The \\a os stream configuration is not modified.\n>>>> */\n>>>> \n>>>> +/**\n>>>> + * \\brief Copy a string with a size limit\n>>>> + * \\param[in] dst The destination string\n>>>> + * \\param[in] src The source string\n>>>> + * \\param[in] size The size of the destination string\n>>>> + *\n>>>> + * strlcpy is available in libbsd, bionic, musl, and ulibc, but not in glibc.\n>>>> + * Instead of checking for strlcpy availability and modifying dependencies,\n>>>> + * it is implemented here as a wrapper around strncpy.\n>>>\n>>> Shouldn't you instead document what the function does ?\n>>>\n>>> * This function copy the null-terminated string \\a src to \\a dst with a limit> * of \\a size - 1 characters, and null-terminates the result if \\a size is\n>>> * larger than 0. If \\a src is larger than \\a size - 1, \\a dst is truncated.\n>>>\n>>\n>> \"Users should check the return value of this call against the size of\n>> the destination buffer to determine if truncation occurs.\" ?\n>>\n>>>> + *\n>>>> + * \\return The size of \\a src\n>>>> + */\n>>>> +size_t strlcpy(char *dst, const char *src, size_t size)\n>>>> +{\n>>>\n>>> \tif (size) {\n>>>\n>>>> +\tstrncpy(dst, src, size);\n>>>> +\tdst[size - 1] = '\\0';\n>>>\n>>> \t}\n>>>\n>>> just to be safe ?\n>>>\n>>> Reviewed-by: Laurent Pinchart \n>>>\n>>>> +\treturn strlen(src);\n>>\n>> So the main thing that caught my eye here was the strlen(src) (rather\n>> than dst) here, which is because strlcpy returns the length of the\n>> source which must be checked against for truncation.\n>>\n>> Should we bring in some compiler attributes to ensure the return value\n>> is always checked?\n>>\n>> https://elixir.bootlin.com/linux/v5.5-rc5/source/include/linux/compiler_types.h#L108\n>> ?\n>>\n>> Saying that, though - I see that the only uses of this call (so far) do\n>> not check the return value or validate against truncation. Perhaps we're\n>> happy with that, but we do so silently.\n>>\n>> Of course in those instances though - we ought to know the length of the\n>> source anyway as it is defined by V4L2 I believe.\n> \n> In this specific case the idea is to indeed truncate. We silently ignore\n> truncation because there's nothing we can do about it, the length of the\n> buffers is defined by the V4L2 APÏ, and they're small enough that\n> truncation happens quite often in existing kernel drivers. This is a bad\n> kernel API, there's not much we can do about it :-(\n\nOk - no problem then.\n--\nKieran\n\n\n\n>>>> +}\n>>>> +\n>>>> } /* namespace utils */\n>>>> \n>>>> } /* namespace libcamera */\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9C1D560612\n\tfor ;\n\tMon, 6 Jan 2020 18:10:21 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D538A52F;\n\tMon, 6 Jan 2020 18:10:20 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578330621;\n\tbh=z3ZPV6Hnked/oqXqmabvkNQ6n5CYLyDNPVYeEP62uuM=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=lURHivd8KNrJZwZCUjaRqBaUo1WWNWiySPs+YblxBuJ00AUPH6/DajqE6BoVL//XV\n\t0TgAYpFfHR6Qzr4pMo0tTvzaHj7PY92uuz6sJlX0vUKZjM5MbGDz8FE11NbC91Wok1\n\tS/kPmAVam8Tfd9Kvj+TwQd3AVc9AXopml+jI1o/Y=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200104001414.12755-1-paul.elder@ideasonboard.com>\n\t<20200104001414.12755-2-paul.elder@ideasonboard.com>\n\t<20200104002237.GH4847@pendragon.ideasonboard.com>\n\t\n\t<20200106170706.GC13373@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<4ff9787a-4269-1213-498a-4071bdb3870e@ideasonboard.com>","Date":"Mon, 6 Jan 2020 17:10:18 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.0","MIME-Version":"1.0","In-Reply-To":"<20200106170706.GC13373@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v6 1/5] libcamera: utils: Add strlcpy","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","X-List-Received-Date":"Mon, 06 Jan 2020 17:10:21 -0000"}}]