Vidar Hasfjord - 3 days ago

Google Gemini suggests:

API Design / Cleanliness: While the fix correctly resolves the crash, relying on nColors as a bit-depth flag (1 << 16, INT_MAX) is an unconventional legacy pattern that can be confusing for maintainers. If backward compatibility permits, a future refactoring could introduce a cleaner overloaded constructor or a dedicated enumeration parameter (e.g., taking an explicit bit-count or format enum) rather than overloading the meaning of nColors.

Something like this, perhaps:

//
/// Used with constructor TDib::TDib(TSize, TBitDepth, int, TPaletteMode).
//
enum class TBitDepth
{ 
  Bpp1 = 1,
  Bpp4 = 4,
  Bpp8 = 8,
  Bpp16 = 16,
  Bpp24 = 24,
  Bpp32 = 32
};

//
/// Used with constructor TDib::TDib(TSize, TBitDepth, int, TPaletteMode).
//
enum class TPaletteMode
{
  Pal = DIB_PAL_COLORS,
  Rgb = DIB_RGB_COLORS
};

//
/// Creates a DIB from the given parameters.
/// 
/// The specified height of the DIB can be negative, in which case a top-down DIB will be created.
/// 
/// The \p paletteSize and \p mode are ignored if the bit-depth is greater than 8. In this case, a
/// high-color DIB is created in which RGB values are stored directly in the image buffer, and no
/// palette (color table) is hence used.
/// 
/// \exception TXOwl is thrown on failure.
///
/// \sa <a href="https://learn.microsoft.com/windows/win32/gdi/device-independent-bitmaps" target="_blank">
/// Device-Independent Bitmaps</a>
//
TDib::TDib(TSize d, TBitDepth b, int paletteSize, TPaletteMode mode)
  : Mode{static_cast<uint16>(mode)}
{
  const auto bitCount = static_cast<WORD>(b);
  const auto maxPaletteSize = NColors(bitCount); // Returns 0 for high-color.
  if (maxPaletteSize < 0) throw TXOwl{_T("TDib: Unsupported bit-depth")};
  WARN(paletteSize < 0 || paletteSize > maxPaletteSize, _T("TDib: Invalid palette size: ") << paletteSize); 
  const auto clrUsed = static_cast<DWORD>(clamp(paletteSize, 0, maxPaletteSize));

  if (d.cx <= 0 || d.cy == 0 || d.cy == LONG_MIN) throw TXOwl{_T("TDib: Invalid dimensions")};
  const auto pitch = ((static_cast<uint64>(d.cx) * bitCount + 31) & ~31) / 8;
  if (!in_range<DWORD>(pitch)) throw TXOwl{_T("TDib: X dimension too large")}; 
  const auto sizeImage = pitch * abs(d.cy);
  if (!in_range<DWORD>(sizeImage)) throw TXOwl{_T("TDib: Dimensions too large")};

  InfoFromHeader({sizeof(BITMAPINFOHEADER),
    d.cx, d.cy,
    1, bitCount, 0, // biPlanes, biBitCount, biCompression
    static_cast<DWORD>(sizeImage),
    0, 0, // biXPelsPerMeter, biYPelsPerMeter
    clrUsed});

  if (clrUsed > 0) switch (mode) // Initialize color table (palette).
  {
  case TPaletteMode::Pal: // Generate a 1:1 palette-relative color table.
    {
      const auto p = reinterpret_cast<uint16*>(Colors); CHECK(p);
      iota(p, p + clrUsed, uint16{0});
    }
    break;

  case TPaletteMode::Rgb: // Copy system palette.
    {
      CHECK(Colors);
      auto palette = vector<PALETTEENTRY>(clrUsed);
      if (auto dc = TScreenDC{}; dc.GetDeviceCaps(RASTERCAPS) & RC_PALETTE)
        dc.GetSystemPaletteEntries(0, clrUsed, data(palette));
      ranges::transform(palette, Colors,
        [](auto e) { return TRgbQuad{e.peRed, e.peGreen, e.peBlue}; });
    }
    break;
  }
}

Gemini review:

Overall, the code is well-protected against integer overflows and invalid dimensions, but there is a notable design inconsistency regarding how palette sizes are handled compared to other errors.

1. Strengths in Validation

  • Dimension & Zero Checks: Checking d.cx <= 0, d.cy == 0, and explicitly guarding against LONG_MIN prevents immediate logical failures and undefined behavior with abs().
  • Overflow Protection: Using uint64 arithmetic for pitch and validating both pitch and sizeImage against in_range<DWORD> successfully prevents buffer overflow vulnerabilities when passing large dimensions to Windows GDI.

2. Areas for Evaluation & Potential Issues

A. Palette Size Handling (WARN vs. throw)

  • Observation: For invalid dimensions or unsupported bit-depths, the constructor throws a TXOwl exception. However, for an invalid paletteSize (e.g., negative or greater than maxPaletteSize), the code uses a WARN(...) macro and silently clamps the value using clamp(paletteSize, 0, maxPaletteSize).
  • Impact: If WARN only logs a message and execution continues, callers passing invalid palette sizes will not experience a hard failure. Depending on your library's error-handling philosophy, this might mask caller bugs rather than enforcing strict validation. If strictness is preferred, out-of-range palette sizes should throw an exception just like invalid dimensions.

B. Enum Cast Safety (TBitDepth & TPaletteMode)

  • Observation: Both parameters rely on strongly-typed enums (enum class), which naturally restricts inputs to defined values. However, if a caller uses a C-style cast or static cast to pass an arbitrary integer (e.g., static_cast<TBitDepth>(999)), NColors(bitCount) must be robust enough to return a negative number or handle the invalid bit depth safely. (Your code does check if (maxPaletteSize < 0) throw TXOwl{...}, which handles this correctly if NColors returns negative for invalid counts).

3. Example of Arguments Causing Non-Throwing Behavior

If a caller passes an out-of-range paletteSize:

  • Example Arguments: TDib(TSize{100, 100}, TBitDepth::Bpp8, 300, TPaletteMode::Rgb) (where Bpp8 allows a maximum palette size of 256).
  • Result: Instead of throwing TXOwl, the constructor triggers the WARN macro, clamps paletteSize to 256, and proceeds to create the DIB.

Recommendation

If silent recovery via clamping is intended behavior, the current implementation is robust and safe. If you want strict validation consistent with the rest of the constructor, replace the WARN macro check with an exception throw:

if (paletteSize < 0 || paletteSize > maxPaletteSize) 
    throw TXOwl{_T("TDib: Invalid palette size")};
 

Last edit: Vidar Hasfjord 1 day ago