[PATCH v2] libcamera: Avoid variable-length arrays
Barnabás Pőcze
pobrn at protonmail.com
Wed Jul 31 00:39:04 CEST 2024
2024. július 31., szerda 0:25 keltezéssel, Laurent Pinchart <laurent.pinchart at ideasonboard.com> írta:
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.
Yes, although my preferred form is
auto p = std::make_unique<T[]>(n);
and yes, I also don't think it changes the performance characteristic too much.
Regards,
Barnabás Pőcze
>
> > > 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