From patchwork Mon Jul 29 18:24:44 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 20718 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 9AA9FBDC71 for ; Mon, 29 Jul 2024 18:25:07 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id A0A5863370; Mon, 29 Jul 2024 20:25:06 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="F+rGa8HM"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 16E4E61984 for ; Mon, 29 Jul 2024 20:25:05 +0200 (CEST) Received: from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi [81.175.209.231]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 58E9245A for ; Mon, 29 Jul 2024 20:24:18 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1722277458; bh=MdTNtz0dvOMfaCjxbvrJaivhVsDApyy2WNByhywvW7w=; h=From:To:Subject:Date:From; b=F+rGa8HMdLrikadV5CjLXUOfKkdppUDI64GNxBBxNHo36+Bwjux7poR7XsJXfGobW h3Vc5n1jZQhsBKimwzMvw49YoW/hO5yux5nGOnXU7hGHFYF+H/tW1sLtXBh3sqze9z /zPJ1HaPOKxb6h86wkUcb+ZmO2r/PHIMnIfv1GWo= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Subject: [PATCH v2] libcamera: Avoid variable-length arrays Date: Mon, 29 Jul 2024 21:24:44 +0300 Message-ID: <20240729182444.16509-1-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.44.2 MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Unlike in C where they have been standardized since C99, variable-length arrays in C++ are an extension supported by gcc and clang. Clang started warning about this in version 18: src/libcamera/ipc_unixsocket.cpp:250:11: error: variable length arrays in C++ are a Clang extension [-Werror,-Wvla-cxx-extension] 250 | char buf[CMSG_SPACE(num * sizeof(uint32_t))]; | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ One simple option is to disable the warning. However, usage of VLAs in C++ is discouraged by some, usually due to security reasons, based on the rationale that developers are often unaware of unintentional use of VLAs and how they may affect the security of the code when the array size is not properly validated. This rationale may sound dubious, as the most commonly proposed fix is to replace VLAs with vectors (or just arrays dynamically allocated with new() wrapped in unique pointers), without adding any size validation. This will not produce much better results. However, keeping the VLA warning and converting the code to dynamic allocation may still be slightly better, as it can prompt developers to notice VLAs and check if size validation is required. For these reasons, convert all VLAs to std::vector. Most of the VLAs don't need extra size validation, as the size is bound through different constraints (e.g. image width for line buffers). An arguable exception may be the buffers in IPCUnixSocket::sendData() and IPCUnixSocket::recvData() as the number of fds is not bound-checked locally, but we will run out of file descriptors before we could overflow the buffer size calculation. Signed-off-by: Laurent Pinchart Reviewed-by: Milan Zamazal Acked-by: Jacopo Mondi --- Changes since v1: - Fix argument to read() in IPC unix socket test --- src/android/jpeg/encoder_libjpeg.cpp | 6 +++--- src/apps/common/dng_writer.cpp | 11 ++++++----- src/apps/common/options.cpp | 8 +++++--- src/ipa/rpi/controller/rpi/alsc.cpp | 6 ++++-- src/libcamera/ipc_unixsocket.cpp | 11 +++++------ test/ipc/unixsocket.cpp | 7 ++++--- 6 files changed, 27 insertions(+), 22 deletions(-) base-commit: c9152bad5ce905f5a31dbd05b40195f02c0cc2a9 diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp index 7fc6287e4bdb..cb242b5ec6a8 100644 --- a/src/android/jpeg/encoder_libjpeg.cpp +++ b/src/android/jpeg/encoder_libjpeg.cpp @@ -125,7 +125,7 @@ void EncoderLibJpeg::compressRGB(const std::vector> &planes) */ void EncoderLibJpeg::compressNV(const std::vector> &planes) { - uint8_t tmprowbuf[compress_.image_width * 3]; + std::vector tmprowbuf(compress_.image_width * 3); /* * \todo Use the raw api, and only unpack the cb/cr samples to new line @@ -149,10 +149,10 @@ void EncoderLibJpeg::compressNV(const std::vector> &planes) const unsigned char *src_c = planes[1].data(); JSAMPROW row_pointer[1]; - row_pointer[0] = &tmprowbuf[0]; + row_pointer[0] = tmprowbuf.data(); for (unsigned int y = 0; y < compress_.image_height; y++) { - unsigned char *dst = &tmprowbuf[0]; + unsigned char *dst = tmprowbuf.data(); const unsigned char *src_y = src + y * y_stride; const unsigned char *src_cb = src_c + (y / vertSubSample) * c_stride + cb_pos; diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp index 355433b08d68..ac4619511677 100644 --- a/src/apps/common/dng_writer.cpp +++ b/src/apps/common/dng_writer.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include @@ -544,7 +545,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, * or a thumbnail scanline. The latter will always be much smaller than * the former as we downscale by 16 in both directions. */ - uint8_t scanline[(config.size.width * info->bitsPerSample + 7) / 8]; + std::vector scanline((config.size.width * info->bitsPerSample + 7) / 8); toff_t rawIFDOffset = 0; toff_t exifIFDOffset = 0; @@ -644,10 +645,10 @@ int DNGWriter::write(const char *filename, const Camera *camera, /* Write the thumbnail. */ const uint8_t *row = static_cast(data); for (unsigned int y = 0; y < config.size.height / 16; y++) { - info->thumbScanline(*info, &scanline, row, + info->thumbScanline(*info, scanline.data(), row, config.size.width / 16, config.stride); - if (TIFFWriteScanline(tif, &scanline, y, 0) != 1) { + if (TIFFWriteScanline(tif, scanline.data(), y, 0) != 1) { std::cerr << "Failed to write thumbnail scanline" << std::endl; TIFFClose(tif); @@ -747,9 +748,9 @@ int DNGWriter::write(const char *filename, const Camera *camera, /* Write RAW content. */ row = static_cast(data); for (unsigned int y = 0; y < config.size.height; y++) { - info->packScanline(&scanline, row, config.size.width); + info->packScanline(scanline.data(), row, config.size.width); - if (TIFFWriteScanline(tif, &scanline, y, 0) != 1) { + if (TIFFWriteScanline(tif, scanline.data(), y, 0) != 1) { std::cerr << "Failed to write RAW scanline" << std::endl; TIFFClose(tif); diff --git a/src/apps/common/options.cpp b/src/apps/common/options.cpp index ab19aa3d48e7..ece268d0e344 100644 --- a/src/apps/common/options.cpp +++ b/src/apps/common/options.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include "options.h" @@ -879,8 +880,8 @@ OptionsParser::Options OptionsParser::parse(int argc, char **argv) * Allocate short and long options arrays large enough to contain all * options. */ - char shortOptions[optionsMap_.size() * 3 + 2]; - struct option longOptions[optionsMap_.size() + 1]; + std::vector shortOptions(optionsMap_.size() * 3 + 2); + std::vector longOptions(optionsMap_.size() + 1); unsigned int ids = 0; unsigned int idl = 0; @@ -922,7 +923,8 @@ OptionsParser::Options OptionsParser::parse(int argc, char **argv) opterr = 0; while (true) { - int c = getopt_long(argc, argv, shortOptions, longOptions, nullptr); + int c = getopt_long(argc, argv, shortOptions.data(), + longOptions.data(), nullptr); if (c == -1) break; diff --git a/src/ipa/rpi/controller/rpi/alsc.cpp b/src/ipa/rpi/controller/rpi/alsc.cpp index 67029fc34d6a..161fd45526ec 100644 --- a/src/ipa/rpi/controller/rpi/alsc.cpp +++ b/src/ipa/rpi/controller/rpi/alsc.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -496,8 +497,9 @@ void resampleCalTable(const Array2D &calTableIn, * Precalculate and cache the x sampling locations and phases to save * recomputing them on every row. */ - int xLo[X], xHi[X]; - double xf[X]; + std::vector xLo(X); + std::vector xHi(X); + std::vector xf(X); double scaleX = cameraMode.sensorWidth / (cameraMode.width * cameraMode.scaleX); double xOff = cameraMode.cropX / (double)cameraMode.sensorWidth; diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp index 75285b679eac..002053e35557 100644 --- a/src/libcamera/ipc_unixsocket.cpp +++ b/src/libcamera/ipc_unixsocket.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include @@ -247,10 +248,9 @@ int IPCUnixSocket::sendData(const void *buffer, size_t length, iov[0].iov_base = const_cast(buffer); iov[0].iov_len = length; - char buf[CMSG_SPACE(num * sizeof(uint32_t))]; - memset(buf, 0, sizeof(buf)); + std::vector buf(CMSG_SPACE(num * sizeof(uint32_t))); - struct cmsghdr *cmsg = (struct cmsghdr *)buf; + struct cmsghdr *cmsg = reinterpret_cast(buf.data()); cmsg->cmsg_len = CMSG_LEN(num * sizeof(uint32_t)); cmsg->cmsg_level = SOL_SOCKET; cmsg->cmsg_type = SCM_RIGHTS; @@ -283,10 +283,9 @@ int IPCUnixSocket::recvData(void *buffer, size_t length, iov[0].iov_base = buffer; iov[0].iov_len = length; - char buf[CMSG_SPACE(num * sizeof(uint32_t))]; - memset(buf, 0, sizeof(buf)); + std::vector buf(CMSG_SPACE(num * sizeof(uint32_t))); - struct cmsghdr *cmsg = (struct cmsghdr *)buf; + struct cmsghdr *cmsg = reinterpret_cast(buf.data()); cmsg->cmsg_len = CMSG_LEN(num * sizeof(uint32_t)); cmsg->cmsg_level = SOL_SOCKET; cmsg->cmsg_type = SCM_RIGHTS; diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp index 2546882da085..f39bd986b8ae 100644 --- a/test/ipc/unixsocket.cpp +++ b/test/ipc/unixsocket.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -340,14 +341,14 @@ protected: for (unsigned int i = 0; i < std::size(strings); i++) { unsigned int len = strlen(strings[i]); - char buf[len]; + std::vector buf(len); close(fds[i]); - if (read(response.fds[0], &buf, len) <= 0) + if (read(response.fds[0], buf.data(), len) <= 0) return TestFail; - if (memcmp(buf, strings[i], len)) + if (memcmp(buf.data(), strings[i], len)) return TestFail; }