[PATCH v2] libcamera: Avoid variable-length arrays
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 31 00:25:46 CEST 2024
On Tue, Jul 30, 2024 at 03:55:23PM +0000, Barnabás Pőcze wrote:
> 2024. július 29., hétfő 20:24 keltezéssel, Laurent Pinchart <laurent.pinchart at ideasonboard.com> írta:
>
> > 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))];
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
>
> As far as I know the warning has existed for a long time, but it was behind
> `-Wvla-extension` for C++ before clang 18, but that seems to be a part of Wpedantic,
> so it was not enabled during normal libcamera builds.
Indeed. I'll update the commit message to "about this with -Wall in
version 18".
> > 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).
>
> Unfortunately it does not appear applicable here, but where Qt is used,
> `QVarLengthArray` can be a nice alternative.
>
> I believe another alternative that is worth considering is the array version
> of unique_ptr.
Do you mean something like
std::unique_ptr<uint8_t[]> tmprowbuf(new uint8_t[compress_.image_width * 3]);
? I've thought about it, but I don't think it really improves runtime
performance much compared to std::vector in the usage patterns below.
> > 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.
>
> The maximum number of file descriptors that can be transferred in a single `sendmsg()`
> call has been 253 for a long time on Linux.
Indeed, another reason why we should be safe.
> > Signed-off-by: Laurent Pinchart <laurent.pinchart 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