Remove or reduce scope of UNSAFE_TODOs in dib code Mostly reduce the scope. Remove a couple of simple ones and replace them with fxcrt::Zip() / using std::array. Unlike a C-array, std::array can do bounds checks when hardened. Bug: 42271176 Change-Id: I8bd5b7ffc26a863f1fa539f0b61892e2fb55e456 Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/121558 Commit-Queue: Lei Zhang <thestig@chromium.org> Reviewed-by: Tom Sepez <tsepez@chromium.org> Reviewed-by: Tom Sepez <tsepez@google.com>
diff --git a/core/fxge/dib/cfx_dibbase.cpp b/core/fxge/dib/cfx_dibbase.cpp index 325ec0d..5f7b423 100644 --- a/core/fxge/dib/cfx_dibbase.cpp +++ b/core/fxge/dib/cfx_dibbase.cpp
@@ -7,6 +7,7 @@ #include "core/fxge/dib/cfx_dibbase.h" #include <algorithm> +#include <array> #include <utility> #include <vector> @@ -22,6 +23,7 @@ #include "core/fxcrt/span.h" #include "core/fxcrt/span_util.h" #include "core/fxcrt/stl_util.h" +#include "core/fxcrt/zip.h" #include "core/fxge/calculate_pitch.h" #include "core/fxge/cfx_cliprgn.h" #include "core/fxge/dib/cfx_bitmapstorer.h" @@ -63,10 +65,10 @@ int bpp = pBitmap->GetBPP() / 8; int width = pBitmap->GetWidth(); int height = pBitmap->GetHeight(); - UNSAFE_TODO({ - for (int row = 0; row < height; ++row) { - pdfium::span<const uint8_t> scan_line = pBitmap->GetScanline(row); - for (int col = 0; col < width; ++col) { + for (int row = 0; row < height; ++row) { + pdfium::span<const uint8_t> scan_line = pBitmap->GetScanline(row); + for (int col = 0; col < width; ++col) { + UNSAFE_TODO({ const uint8_t* src_port = scan_line.subspan(Fx2DSizeOrDie(col, bpp)).data(); uint32_t b = src_port[0] & 0xf0; @@ -74,22 +76,24 @@ uint32_t r = src_port[2] & 0xf0; uint32_t index = (r << 4) + g + (b >> 4); ++m_Luts[index].first; - } + }); } - // Move non-zeros to the front and count them - for (uint32_t row = 0; row < m_Luts.size(); ++row) { + } + // Move non-zeros to the front and count them + for (uint32_t row = 0; row < m_Luts.size(); ++row) { + UNSAFE_TODO({ if (m_Luts[row].first != 0) { m_Luts[m_lut].first = m_Luts[row].first; m_Luts[m_lut].second = row; ++m_lut; } - } - pdfium::span<LutsData> lut_span = pdfium::make_span(m_Luts).first(m_lut); - std::sort(lut_span.begin(), lut_span.end(), - [](const LutsData& arg1, const LutsData& arg2) { - return arg1.first < arg2.first; - }); - }); + }); + } + pdfium::span<LutsData> lut_span = pdfium::make_span(m_Luts).first(m_lut); + std::sort(lut_span.begin(), lut_span.end(), + [](const LutsData& arg1, const LutsData& arg2) { + return arg1.first < arg2.first; + }); ObtainPalette(); } @@ -213,26 +217,22 @@ int dest_pitch, int width, int height, - const RetainPtr<const CFX_DIBBase>& pSrcBitmap, + const RetainPtr<const CFX_DIBBase>& src, int src_left, int src_top) { - pdfium::span<const uint32_t> src_palette = pSrcBitmap->GetPaletteSpan(); - uint8_t gray[256]; - UNSAFE_TODO({ - for (size_t i = 0; i < std::size(gray); ++i) { - gray[i] = FXRGB2GRAY(FXARGB_R(src_palette[i]), FXARGB_G(src_palette[i]), - FXARGB_B(src_palette[i])); + pdfium::span<const uint32_t> src_palette = src->GetPaletteSpan(); + CHECK_EQ(256u, src_palette.size()); + std::array<uint8_t, 256> gray; + for (auto [input, output] : fxcrt::Zip(src_palette, gray)) { + output = FXRGB2GRAY(FXARGB_R(input), FXARGB_G(input), FXARGB_B(input)); + } + for (int row = 0; row < height; ++row) { + auto dest_scan = dest_buf.subspan(Fx2DSizeOrDie(row, dest_pitch)); + auto src_scan = src->GetScanline(src_top + row).subspan(src_left, width); + for (auto [input, output] : fxcrt::Zip(src_scan, dest_scan)) { + output = gray[input]; } - for (int row = 0; row < height; ++row) { - uint8_t* dest_scan = - dest_buf.subspan(Fx2DSizeOrDie(row, dest_pitch)).data(); - const uint8_t* src_scan = - pSrcBitmap->GetScanline(src_top + row).subspan(src_left).data(); - for (int col = 0; col < width; ++col) { - *dest_scan++ = gray[*src_scan++]; - } - } - }); + } } void ConvertBuffer_Rgb2Gray(pdfium::span<uint8_t> dest_buf, @@ -438,6 +438,7 @@ int src_left, int src_top) { pdfium::span<const uint32_t> src_palette = pSrcBitmap->GetPaletteSpan(); + CHECK_EQ(256u, src_palette.size()); uint8_t dst_palette[768]; UNSAFE_TODO({ for (int i = 0; i < 256; ++i) { @@ -445,19 +446,21 @@ dst_palette[3 * i + 1] = FXARGB_G(src_palette[i]); dst_palette[3 * i + 2] = FXARGB_R(src_palette[i]); } - int comps = GetCompsFromFormat(dest_format); - for (int row = 0; row < height; ++row) { - uint8_t* dest_scan = - dest_buf.subspan(Fx2DSizeOrDie(row, dest_pitch)).data(); - const uint8_t* src_scan = - pSrcBitmap->GetScanline(src_top + row).subspan(src_left).data(); - for (int col = 0; col < width; ++col) { + }); + const int comps = GetCompsFromFormat(dest_format); + for (int row = 0; row < height; ++row) { + uint8_t* dest_scan = + dest_buf.subspan(Fx2DSizeOrDie(row, dest_pitch)).data(); + const uint8_t* src_scan = + pSrcBitmap->GetScanline(src_top + row).subspan(src_left).data(); + for (int col = 0; col < width; ++col) { + UNSAFE_TODO({ uint8_t* src_pixel = dst_palette + 3 * (*src_scan++); FXSYS_memcpy(dest_scan, src_pixel, 3); dest_scan += comps; - } + }); } - }); + } } void ConvertBuffer_24bppRgb2Rgb24(
diff --git a/core/fxge/dib/cfx_dibitmap.cpp b/core/fxge/dib/cfx_dibitmap.cpp index e687048..6d4be64 100644 --- a/core/fxge/dib/cfx_dibitmap.cpp +++ b/core/fxge/dib/cfx_dibitmap.cpp
@@ -443,9 +443,9 @@ } return; } - UNSAFE_TODO({ - if (forecolor == 0 && backcolor == 0xffffff) { - for (int row = 0; row < GetHeight(); ++row) { + if (forecolor == 0 && backcolor == 0xffffff) { + for (int row = 0; row < GetHeight(); ++row) { + UNSAFE_TODO({ uint8_t* scanline = m_pBuffer.Get() + row * GetPitch(); int gap = GetBPP() / 8 - 2; for (int col = 0; col < GetWidth(); ++col) { @@ -455,10 +455,12 @@ *scanline = gray; scanline += gap; } - } - return; + }); } - for (int row = 0; row < GetHeight(); ++row) { + return; + } + for (int row = 0; row < GetHeight(); ++row) { + UNSAFE_TODO({ uint8_t* scanline = m_pBuffer.Get() + row * GetPitch(); int gap = GetBPP() / 8 - 2; for (int col = 0; col < GetWidth(); ++col) { @@ -468,8 +470,8 @@ *scanline = br + (fr - br) * gray / 255; scanline += gap; } - } - }); + }); + } } bool CFX_DIBitmap::ConvertColorScale(uint32_t forecolor, uint32_t backcolor) { @@ -690,8 +692,8 @@ width = rect.Width(); uint32_t dst_color = color; uint8_t* color_p = reinterpret_cast<uint8_t*>(&dst_color); - UNSAFE_TODO({ - if (GetBPP() == 8) { + if (GetBPP() == 8) { + UNSAFE_TODO({ uint8_t gray = IsMaskFormat() ? 255 @@ -707,9 +709,11 @@ } } } - return true; - } - if (GetBPP() == 1) { + }); + return true; + } + if (GetBPP() == 1) { + UNSAFE_TODO({ int left_shift = rect.left % 8; int right_shift = rect.right % 8; int new_width = rect.right / 8 - rect.left / 8; @@ -747,19 +751,21 @@ } } } - return true; - } + }); + return true; + } - CHECK_GE(GetBPP(), 24); - color_p[3] = static_cast<uint8_t>(src_alpha); - int Bpp = GetBPP() / 8; - const bool bAlpha = IsAlphaFormat(); - if (bAlpha) { - // Other formats with alpha have already been handled above. - DCHECK_EQ(GetFormat(), FXDIB_Format::kArgb); - } - if (src_alpha == 255) { - for (int row = rect.top; row < rect.bottom; row++) { + CHECK_GE(GetBPP(), 24); + UNSAFE_TODO({ color_p[3] = static_cast<uint8_t>(src_alpha); }); + int Bpp = GetBPP() / 8; + const bool bAlpha = IsAlphaFormat(); + if (bAlpha) { + // Other formats with alpha have already been handled above. + DCHECK_EQ(GetFormat(), FXDIB_Format::kArgb); + } + if (src_alpha == 255) { + for (int row = rect.top; row < rect.bottom; row++) { + UNSAFE_TODO({ uint8_t* dest_scan = m_pBuffer.Get() + row * GetPitch() + rect.left * Bpp; if (Bpp == 4) { @@ -774,10 +780,12 @@ *dest_scan++ = color_p[2]; } } - } - return true; + }); } - for (int row = rect.top; row < rect.bottom; row++) { + return true; + } + for (int row = rect.top; row < rect.bottom; row++) { + UNSAFE_TODO({ uint8_t* dest_scan = m_pBuffer.Get() + row * GetPitch() + rect.left * Bpp; if (bAlpha) { for (int col = 0; col < width; col++) { @@ -812,8 +820,8 @@ } } } - } - }); + }); + } return true; }