[PATCH v2] libcamera: Avoid variable-length arrays
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Jul 30 14:48:05 CEST 2024
Hi Laurent
On Mon, Jul 29, 2024 at 09:24:44PM GMT, Laurent Pinchart wrote:
> 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 <laurent.pinchart at ideasonboard.com>
Acked-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> ---
> 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(-)
>
> 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<Span<uint8_t>> &planes)
> */
> void EncoderLibJpeg::compressNV(const std::vector<Span<uint8_t>> &planes)
> {
> - uint8_t tmprowbuf[compress_.image_width * 3];
> + std::vector<uint8_t> 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<Span<uint8_t>> &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 <endian.h>
> #include <iostream>
> #include <map>
> +#include <vector>
>
> #include <tiffio.h>
>
> @@ -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<uint8_t> 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<const uint8_t *>(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<const uint8_t *>(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 <iomanip>
> #include <iostream>
> #include <string.h>
> +#include <vector>
>
> #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<char> shortOptions(optionsMap_.size() * 3 + 2);
> + std::vector<struct option> 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 <functional>
> #include <math.h>
> #include <numeric>
> +#include <vector>
>
> #include <libcamera/base/log.h>
> #include <libcamera/base/span.h>
> @@ -496,8 +497,9 @@ void resampleCalTable(const Array2D<double> &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<int> xLo(X);
> + std::vector<int> xHi(X);
> + std::vector<double> 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 <string.h>
> #include <sys/socket.h>
> #include <unistd.h>
> +#include <vector>
>
> #include <libcamera/base/event_notifier.h>
> #include <libcamera/base/log.h>
> @@ -247,10 +248,9 @@ int IPCUnixSocket::sendData(const void *buffer, size_t length,
> iov[0].iov_base = const_cast<void *>(buffer);
> iov[0].iov_len = length;
>
> - char buf[CMSG_SPACE(num * sizeof(uint32_t))];
> - memset(buf, 0, sizeof(buf));
> + std::vector<uint8_t> buf(CMSG_SPACE(num * sizeof(uint32_t)));
>
> - struct cmsghdr *cmsg = (struct cmsghdr *)buf;
> + struct cmsghdr *cmsg = reinterpret_cast<struct cmsghdr *>(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<uint8_t> buf(CMSG_SPACE(num * sizeof(uint32_t)));
>
> - struct cmsghdr *cmsg = (struct cmsghdr *)buf;
> + struct cmsghdr *cmsg = reinterpret_cast<struct cmsghdr *>(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 <sys/types.h>
> #include <sys/wait.h>
> #include <unistd.h>
> +#include <vector>
>
> #include <libcamera/base/event_dispatcher.h>
> #include <libcamera/base/thread.h>
> @@ -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<char> 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;
> }
>
>
> base-commit: c9152bad5ce905f5a31dbd05b40195f02c0cc2a9
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list