Avoid using FX_SAFE_* types in other header files.
By necessity, this drags in all of the safe_numerics arithmetic
headers, which are quite large. In one place, we use Optional<>
(since optional.h is much much smaller) to return a non-result,
converting back to the safe cases only in .cpp files.
-- rename member to ensure all cases are caught.
-- use const Optional<> locals where possible to prevent additional
arithmetic against the results which may be unsafe.
-- add missing include in fuzzer.
Change-Id: Id506d8aee0376d2bf343d80b0e2992b4fe3e3a74
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/83511
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
diff --git a/core/fpdfapi/page/cpdf_dib.cpp b/core/fpdfapi/page/cpdf_dib.cpp
index 996a856..9b88d50 100644
--- a/core/fpdfapi/page/cpdf_dib.cpp
+++ b/core/fpdfapi/page/cpdf_dib.cpp
@@ -181,8 +181,13 @@
if (m_bDoBpcCheck && (m_bpc == 0 || m_nComponents == 0))
return false;
- FX_SAFE_UINT32 src_size =
- fxcodec::CalculatePitch8(m_bpc, m_nComponents, m_Width) * m_Height;
+ const Optional<uint32_t> maybe_size =
+ fxcodec::CalculatePitch8(m_bpc, m_nComponents, m_Width);
+ if (!maybe_size.has_value())
+ return false;
+
+ FX_SAFE_UINT32 src_size = maybe_size.value();
+ src_size *= m_Height;
if (!src_size.IsValid())
return false;
@@ -199,22 +204,22 @@
else
m_Format = MakeRGBFormat(CalculateBitsPerPixel(m_bpc, m_nComponents));
- FX_SAFE_UINT32 pitch =
+ Optional<uint32_t> pitch =
fxcodec::CalculatePitch32(GetBppFromFormat(m_Format), m_Width);
- if (!pitch.IsValid())
+ if (!pitch.has_value())
return false;
- m_pLineBuf.reset(FX_Alloc(uint8_t, pitch.ValueOrDie()));
+ m_pLineBuf.reset(FX_Alloc(uint8_t, pitch.value()));
LoadPalette();
if (m_bColorKey) {
m_Format = FXDIB_Format::kArgb;
pitch = fxcodec::CalculatePitch32(GetBppFromFormat(m_Format), m_Width);
- if (!pitch.IsValid())
+ if (!pitch.has_value())
return false;
- m_pMaskedLine.reset(FX_Alloc(uint8_t, pitch.ValueOrDie()));
+ m_pMaskedLine.reset(FX_Alloc(uint8_t, pitch.value()));
}
- m_Pitch = pitch.ValueOrDie();
+ m_Pitch = pitch.value();
return true;
}
@@ -228,12 +233,12 @@
m_Format = MakeRGBFormat(CalculateBitsPerPixel(m_bpc, m_nComponents));
}
- FX_SAFE_UINT32 pitch =
+ Optional<uint32_t> pitch =
fxcodec::CalculatePitch32(GetBppFromFormat(m_Format), m_Width);
- if (!pitch.IsValid())
+ if (!pitch.has_value())
return false;
- m_pLineBuf.reset(FX_Alloc(uint8_t, pitch.ValueOrDie()));
+ m_pLineBuf.reset(FX_Alloc(uint8_t, pitch.value()));
if (m_pColorSpace && m_bStdCS) {
m_pColorSpace->EnableStdConversion(true);
}
@@ -241,11 +246,11 @@
if (m_bColorKey) {
m_Format = FXDIB_Format::kArgb;
pitch = fxcodec::CalculatePitch32(GetBppFromFormat(m_Format), m_Width);
- if (!pitch.IsValid())
+ if (!pitch.has_value())
return false;
- m_pMaskedLine.reset(FX_Alloc(uint8_t, pitch.ValueOrDie()));
+ m_pMaskedLine.reset(FX_Alloc(uint8_t, pitch.value()));
}
- m_Pitch = pitch.ValueOrDie();
+ m_Pitch = pitch.value();
return true;
}
@@ -280,8 +285,13 @@
if (m_bDoBpcCheck && (m_bpc == 0 || m_nComponents == 0))
return LoadState::kFail;
- FX_SAFE_UINT32 src_size =
- fxcodec::CalculatePitch8(m_bpc, m_nComponents, m_Width) * m_Height;
+ const Optional<uint32_t> maybe_size =
+ fxcodec::CalculatePitch8(m_bpc, m_nComponents, m_Width);
+ if (!maybe_size.has_value())
+ return LoadState::kFail;
+
+ FX_SAFE_UINT32 src_size = maybe_size.value();
+ src_size *= m_Height;
if (!src_size.IsValid())
return LoadState::kFail;
@@ -540,15 +550,15 @@
if (!m_pDecoder)
return LoadState::kFail;
- FX_SAFE_UINT32 requested_pitch =
+ const Optional<uint32_t> requested_pitch =
fxcodec::CalculatePitch8(m_bpc, m_nComponents, m_Width);
- if (!requested_pitch.IsValid())
+ if (!requested_pitch.has_value())
return LoadState::kFail;
- FX_SAFE_UINT32 provided_pitch = fxcodec::CalculatePitch8(
+ const Optional<uint32_t> provided_pitch = fxcodec::CalculatePitch8(
m_pDecoder->GetBPC(), m_pDecoder->CountComps(), m_pDecoder->GetWidth());
- if (!provided_pitch.IsValid())
+ if (!provided_pitch.has_value())
return LoadState::kFail;
- if (provided_pitch.ValueOrDie() < requested_pitch.ValueOrDie())
+ if (provided_pitch.value() < requested_pitch.value())
return LoadState::kFail;
return LoadState::kSuccess;
}
@@ -1066,12 +1076,12 @@
if (m_bpc == 0)
return nullptr;
- FX_SAFE_UINT32 src_pitch =
+ const Optional<uint32_t> src_pitch =
fxcodec::CalculatePitch8(m_bpc, m_nComponents, m_Width);
- if (!src_pitch.IsValid())
+ if (!src_pitch.has_value())
return nullptr;
- uint32_t src_pitch_value = src_pitch.ValueOrDie();
+ uint32_t src_pitch_value = src_pitch.value();
const uint8_t* pSrcLine = nullptr;
if (m_pCachedBitmap && src_pitch_value <= m_pCachedBitmap->GetPitch()) {
if (line >= m_pCachedBitmap->GetHeight()) {
diff --git a/core/fpdfapi/page/cpdf_streamparser.cpp b/core/fpdfapi/page/cpdf_streamparser.cpp
index 4b5e1ca..707b0b2 100644
--- a/core/fpdfapi/page/cpdf_streamparser.cpp
+++ b/core/fpdfapi/page/cpdf_streamparser.cpp
@@ -52,7 +52,11 @@
if (width <= 0 || height <= 0)
return FX_INVALID_OFFSET;
- FX_SAFE_UINT32 size = fxcodec::CalculatePitch8(bpc, ncomps, width);
+ Optional<uint32_t> maybe_size = fxcodec::CalculatePitch8(bpc, ncomps, width);
+ if (!maybe_size.has_value())
+ return FX_INVALID_OFFSET;
+
+ FX_SAFE_UINT32 size = maybe_size.value();
size *= height;
if (size.ValueOrDefault(0) == 0)
return FX_INVALID_OFFSET;
@@ -158,7 +162,12 @@
nComponents = pCS ? pCS->CountComponents() : 3;
bpc = pDict->GetIntegerFor("BitsPerComponent");
}
- FX_SAFE_UINT32 size = fxcodec::CalculatePitch8(bpc, nComponents, width);
+ Optional<uint32_t> maybe_size =
+ fxcodec::CalculatePitch8(bpc, nComponents, width);
+ if (!maybe_size.has_value())
+ return nullptr;
+
+ FX_SAFE_UINT32 size = maybe_size.value();
size *= height;
if (!size.IsValid())
return nullptr;
diff --git a/core/fxcodec/bmp/cfx_bmpdecompressor.cpp b/core/fxcodec/bmp/cfx_bmpdecompressor.cpp
index eaa8f67..3b9070c 100644
--- a/core/fxcodec/bmp/cfx_bmpdecompressor.cpp
+++ b/core/fxcodec/bmp/cfx_bmpdecompressor.cpp
@@ -227,27 +227,27 @@
default:
return BmpDecoder::Status::kFail;
}
- FX_SAFE_UINT32 stride = CalculatePitch32(bit_counts_, width_);
- if (!stride.IsValid())
+ Optional<uint32_t> stride = CalculatePitch32(bit_counts_, width_);
+ if (!stride.has_value())
return BmpDecoder::Status::kFail;
- src_row_bytes_ = stride.ValueOrDie();
+ src_row_bytes_ = stride.value();
switch (bit_counts_) {
case 1:
case 4:
case 8:
stride = CalculatePitch32(8, width_);
- if (!stride.IsValid())
+ if (!stride.has_value())
return BmpDecoder::Status::kFail;
- out_row_bytes_ = stride.ValueOrDie();
+ out_row_bytes_ = stride.value();
components_ = 1;
break;
case 16:
case 24:
stride = CalculatePitch32(24, width_);
- if (!stride.IsValid())
+ if (!stride.has_value())
return BmpDecoder::Status::kFail;
- out_row_bytes_ = stride.ValueOrDie();
+ out_row_bytes_ = stride.value();
components_ = 3;
break;
case 32:
diff --git a/core/fxcodec/fax/faxmodule.cpp b/core/fxcodec/fax/faxmodule.cpp
index bb07b13..7e72594 100644
--- a/core/fxcodec/fax/faxmodule.cpp
+++ b/core/fxcodec/fax/faxmodule.cpp
@@ -497,7 +497,7 @@
height,
kFaxComps,
kFaxBpc,
- CalculatePitch32(kFaxBpc, width).ValueOrDie()),
+ CalculatePitch32OrDie(kFaxBpc, width)),
m_Encoding(K),
m_bByteAlign(EncodedByteAlign),
m_bEndOfLine(EndOfLine),
diff --git a/core/fxcodec/flate/flatemodule.cpp b/core/fxcodec/flate/flatemodule.cpp
index 2c2101e..316e02d 100644
--- a/core/fxcodec/flate/flatemodule.cpp
+++ b/core/fxcodec/flate/flatemodule.cpp
@@ -16,6 +16,7 @@
#include "core/fxcodec/scanlinedecoder.h"
#include "core/fxcrt/fx_extension.h"
#include "core/fxcrt/fx_memory_wrappers.h"
+#include "core/fxcrt/fx_safe_types.h"
#include "third_party/base/check.h"
#include "third_party/base/notreached.h"
#include "third_party/base/numerics/safe_conversions.h"
@@ -296,7 +297,7 @@
int bpc,
int nColors,
int nPixels) {
- const uint32_t row_size = CalculatePitch8(bpc, nColors, nPixels).ValueOrDie();
+ const uint32_t row_size = CalculatePitch8OrDie(bpc, nColors, nPixels);
const uint32_t BytesPerPixel = (bpc * nColors + 7) / 8;
uint8_t tag = pSrcData[0];
if (tag == 0) {
@@ -622,7 +623,7 @@
height,
nComps,
bpc,
- CalculatePitch8(bpc, nComps, width).ValueOrDie()),
+ CalculatePitch8OrDie(bpc, nComps, width)),
m_SrcBuf(src_span),
m_pScanline(FX_Alloc(uint8_t, m_Pitch)) {}
@@ -700,7 +701,7 @@
m_BitsPerComponent = BitsPerComponent;
m_Columns = Columns;
m_PredictPitch =
- CalculatePitch8(m_BitsPerComponent, m_Colors, m_Columns).ValueOrDie();
+ CalculatePitch8OrDie(m_BitsPerComponent, m_Colors, m_Columns);
m_LastLine.resize(m_PredictPitch);
m_PredictBuffer.resize(m_PredictPitch);
m_PredictRaw.resize(m_PredictPitch + 1);
diff --git a/core/fxcodec/fx_codec.cpp b/core/fxcodec/fx_codec.cpp
index 497646d..c94eb71 100644
--- a/core/fxcodec/fx_codec.cpp
+++ b/core/fxcodec/fx_codec.cpp
@@ -10,9 +10,33 @@
#include <utility>
#include "core/fxcrt/fx_memory.h"
+#include "core/fxcrt/fx_safe_types.h"
#include "core/fxge/dib/fx_dib.h"
namespace fxcodec {
+namespace {
+
+FX_SAFE_UINT32 CalculatePitch8Safely(uint32_t bpc,
+ uint32_t components,
+ int width) {
+ FX_SAFE_UINT32 pitch = bpc;
+ pitch *= components;
+ pitch *= width;
+ pitch += 7;
+ pitch /= 8;
+ return pitch;
+}
+
+FX_SAFE_UINT32 CalculatePitch32Safely(int bpp, int width) {
+ FX_SAFE_UINT32 pitch = bpp;
+ pitch *= width;
+ pitch += 31;
+ pitch /= 32; // quantized to number of 32-bit words.
+ pitch *= 4; // and then back to bytes, (not just /8 in one step).
+ return pitch;
+}
+
+} // namespace
#ifdef PDF_ENABLE_XFA
CFX_DIBAttribute::CFX_DIBAttribute() = default;
@@ -38,22 +62,28 @@
}
}
-FX_SAFE_UINT32 CalculatePitch8(uint32_t bpc, uint32_t components, int width) {
- FX_SAFE_UINT32 pitch = bpc;
- pitch *= components;
- pitch *= width;
- pitch += 7;
- pitch /= 8;
- return pitch;
+uint32_t CalculatePitch8OrDie(uint32_t bpc, uint32_t components, int width) {
+ return CalculatePitch8Safely(bpc, components, width).ValueOrDie();
}
-FX_SAFE_UINT32 CalculatePitch32(int bpp, int width) {
- FX_SAFE_UINT32 pitch = bpp;
- pitch *= width;
- pitch += 31;
- pitch /= 32; // quantized to number of 32-bit words.
- pitch *= 4; // and then back to bytes, (not just /8 in one step).
- return pitch;
+uint32_t CalculatePitch32OrDie(int bpp, int width) {
+ return CalculatePitch32Safely(bpp, width).ValueOrDie();
+}
+
+Optional<uint32_t> CalculatePitch8(uint32_t bpc,
+ uint32_t components,
+ int width) {
+ FX_SAFE_UINT32 pitch = CalculatePitch8Safely(bpc, components, width);
+ if (!pitch.IsValid())
+ return pdfium::nullopt;
+ return pitch.ValueOrDie();
+}
+
+Optional<uint32_t> CalculatePitch32(int bpp, int width) {
+ FX_SAFE_UINT32 pitch = CalculatePitch32Safely(bpp, width);
+ if (!pitch.IsValid())
+ return pdfium::nullopt;
+ return pitch.ValueOrDie();
}
} // namespace fxcodec
diff --git a/core/fxcodec/fx_codec.h b/core/fxcodec/fx_codec.h
index f84e752..379314b 100644
--- a/core/fxcodec/fx_codec.h
+++ b/core/fxcodec/fx_codec.h
@@ -9,7 +9,7 @@
#include <map>
-#include "core/fxcrt/fx_safe_types.h"
+#include "third_party/base/optional.h"
namespace fxcodec {
@@ -28,8 +28,12 @@
void ReverseRGB(uint8_t* pDestBuf, const uint8_t* pSrcBuf, int pixels);
-FX_SAFE_UINT32 CalculatePitch8(uint32_t bpc, uint32_t components, int width);
-FX_SAFE_UINT32 CalculatePitch32(int bpp, int width);
+uint32_t CalculatePitch8OrDie(uint32_t bpc, uint32_t components, int width);
+uint32_t CalculatePitch32OrDie(int bpp, int width);
+Optional<uint32_t> CalculatePitch8(uint32_t bpc,
+ uint32_t components,
+ int width);
+Optional<uint32_t> CalculatePitch32(int bpp, int width);
} // namespace fxcodec
diff --git a/core/fxcodec/jbig2/JBig2_Context.cpp b/core/fxcodec/jbig2/JBig2_Context.cpp
index 351bc86..083e95b 100644
--- a/core/fxcodec/jbig2/JBig2_Context.cpp
+++ b/core/fxcodec/jbig2/JBig2_Context.cpp
@@ -91,7 +91,7 @@
m_pSegment.reset();
return nRet;
}
- m_dwOffset = m_pStream->getOffset();
+ m_nOffset = m_pStream->getOffset();
}
nRet = ParseSegmentData(m_pSegment.get(), pPause);
if (m_ProcessingStatus == FXCODEC_STATUS_DECODE_TOBECONTINUE) {
@@ -108,11 +108,12 @@
return nRet;
}
if (m_pSegment->m_dwData_length != 0xffffffff) {
- m_dwOffset += m_pSegment->m_dwData_length;
- if (!m_dwOffset.IsValid())
+ FX_SAFE_UINT32 new_offset = m_nOffset;
+ new_offset += m_pSegment->m_dwData_length;
+ if (!new_offset.IsValid())
return JBig2_Result::kFailure;
-
- m_pStream->setOffset(m_dwOffset.ValueOrDie());
+ m_nOffset = new_offset.ValueOrDie();
+ m_pStream->setOffset(m_nOffset);
} else {
m_pStream->offset(4);
}
diff --git a/core/fxcodec/jbig2/JBig2_Context.h b/core/fxcodec/jbig2/JBig2_Context.h
index 11d104e..1375986 100644
--- a/core/fxcodec/jbig2/JBig2_Context.h
+++ b/core/fxcodec/jbig2/JBig2_Context.h
@@ -15,7 +15,6 @@
#include "core/fxcodec/fx_codec_def.h"
#include "core/fxcodec/jbig2/JBig2_Page.h"
#include "core/fxcodec/jbig2/JBig2_Segment.h"
-#include "core/fxcrt/fx_safe_types.h"
#include "core/fxcrt/unowned_ptr.h"
#include "third_party/base/span.h"
@@ -101,7 +100,7 @@
std::unique_ptr<CJBig2_ArithDecoder> m_pArithDecoder;
std::unique_ptr<CJBig2_GRDProc> m_pGRD;
std::unique_ptr<CJBig2_Segment> m_pSegment;
- FX_SAFE_UINT32 m_dwOffset = 0;
+ uint32_t m_nOffset = 0;
JBig2RegionInfo m_ri;
UnownedPtr<std::list<CJBig2_CachePair>> const m_pSymbolDictCache;
};
diff --git a/testing/fuzzers/xfa_codec_fuzzer.h b/testing/fuzzers/xfa_codec_fuzzer.h
index 2979710..8b9d575 100644
--- a/testing/fuzzers/xfa_codec_fuzzer.h
+++ b/testing/fuzzers/xfa_codec_fuzzer.h
@@ -10,6 +10,7 @@
#include "core/fxcodec/fx_codec.h"
#include "core/fxcodec/progressive_decoder.h"
#include "core/fxcrt/cfx_readonlymemorystream.h"
+#include "core/fxcrt/fx_safe_types.h"
#include "core/fxcrt/retain_ptr.h"
#include "core/fxge/dib/cfx_dibitmap.h"
#include "third_party/base/span.h"