[v2] libcamera: Avoid variable-length arrays

20718 diff mbox series

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@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(-)


base-commit: c9152bad5ce905f5a31dbd05b40195f02c0cc2a9

Hi Laurent,

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> 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.

Hmm, your argumentation is convincing and I don't have any better idea
than to accept using std::vector::data() in the given places.

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

> Signed-off-by: Laurent Pinchart <laurent.pinchart@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

Hi Milan,

On Tue, Jul 30, 2024 at 09:43:58AM +0200, Milan Zamazal wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> 
> > 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.
> 
> Hmm, your argumentation is convincing and I don't have any better idea
> than to accept using std::vector::data() in the given places.

I'm not entirely thrilled by this, as I think VLAs are fine in this
case, but the dynamic heap allocation with std::vector shouldn't be a
big burden in any of the code paths below.

> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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
>

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@ideasonboard.com>

Acked-by: Jacopo Mondi <jacopo.mondi@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
>

2024. július 29., hétfő 20:24 keltezéssel, Laurent Pinchart <laurent.pinchart@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.


> 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.


> 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.


Regards,
Barnabás Pőcze


> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@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
>

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@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@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

2024. július 31., szerda 0:25 keltezéssel, Laurent Pinchart <laurent.pinchart@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@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@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
>