BMPs from the clipboard may include extra padding.

In the original Windows clipboard BMP decoder implementation in
nsImageFromClipboard::ConvertColorBitMap, if the bitmap used bitfields
compression, it always adjusted the offset to the RGB data by 12 bytes.
It did this even for newer BMP header formats which explicitly include
space for the bitfields in their header sizes. This patch updates our
BMP decoder to do the same for clipboard BMPs, since we have observed
pasted BMPs using bitfield compression appearing incorrectly. To the
user this appears as if we read a color mask; completely red, blue,
green pixels at the start of the last row, causing all of the other rows
to start with the last three pixels of the previous row.

2 files changed, 36 insertions, 16 deletions

diff --git a/image/decoders/nsBMPDecoder.cpp b/image/decoders/nsBMPDecoder.cpp
index fc79c223d..b93eb42b6 100644
--- a/image/decoders/nsBMPDecoder.cpp
+++ b/image/decoders/nsBMPDecoder.cpp
@@ -66,6 +66,17 @@
 //   compression, then instead of treating the pixel data as 0RGB it is treated
 //   as ARGB, but only if one or more of the A values are non-zero.
 //
+// Clipboard variants.
+// - It's the BMP format used for BMP images captured from the clipboard.
+// - It is missing the file header, containing the BM signature and the data
+//   offset. Instead the data begins after the header.
+// - If it uses BITFIELDS compression, then there is always an additional 12
+//   bytes of data after the header that must be read. In WinBMPv4+, the masks
+//   are supposed to be included in the header size, which are the values we use
+//   for decoding purposes, but there is additional three masks following the
+//   header which must be skipped to get to the pixel data.
+//
+//
 // OS/2 VERSIONS OF THE BMP FORMAT
 // -------------------------------
 // OS2-BMPv1.
@@ -168,10 +179,12 @@ static mozilla::LazyLogModule sBMPLog("BMPDecoder");
 // The length of the mBIHSize field in the info header.
 static const uint32_t BIHSIZE_FIELD_LENGTH = 4;
 
-nsBMPDecoder::nsBMPDecoder(RasterImage* aImage, State aState, size_t aLength)
+nsBMPDecoder::nsBMPDecoder(RasterImage* aImage, State aState, size_t aLength,
+                           bool aForClipboard)
   : Decoder(aImage)
   , mLexer(Transition::To(aState, aLength), Transition::TerminateSuccess())
   , mIsWithinICO(false)
+  , mIsForClipboard(aForClipboard)
   , mMayHaveTransparency(false)
   , mDoesHaveTransparency(false)
   , mNumColors(0)
@@ -188,14 +201,16 @@ nsBMPDecoder::nsBMPDecoder(RasterImage* aImage, State aState, size_t aLength)
 // Constructor for normal BMP files or from the clipboard.
 nsBMPDecoder::nsBMPDecoder(RasterImage* aImage, bool aForClipboard)
   : nsBMPDecoder(aImage,
-                 aForClipboard ? State::CLIPBOARD_HEADER : State::FILE_HEADER,
-                 aForClipboard ? BIHSIZE_FIELD_LENGTH : FILE_HEADER_LENGTH)
+                 aForClipboard ? State::INFO_HEADER_SIZE : State::FILE_HEADER,
+                 aForClipboard ? BIHSIZE_FIELD_LENGTH : FILE_HEADER_LENGTH,
+                 aForClipboard)
 {
 }
 
 // Constructor used for WinBMPv3-ICO files, which lack a file header.
 nsBMPDecoder::nsBMPDecoder(RasterImage* aImage, uint32_t aDataOffset)
-  : nsBMPDecoder(aImage, State::INFO_HEADER_SIZE, BIHSIZE_FIELD_LENGTH)
+  : nsBMPDecoder(aImage, State::INFO_HEADER_SIZE, BIHSIZE_FIELD_LENGTH,
+                 /* aForClipboard */ false)
 {
   SetIsWithinICO();
 
@@ -457,7 +472,6 @@ nsBMPDecoder::DoDecode(SourceBufferIterator& aIterator, IResumable* aOnResume)
                     [=](State aState, const char* aData, size_t aLength) {
     switch (aState) {
       case State::FILE_HEADER:      return ReadFileHeader(aData, aLength);
-      case State::CLIPBOARD_HEADER: return ReadClipboardHeader(aData, aLength);
       case State::INFO_HEADER_SIZE: return ReadInfoHeaderSize(aData, aLength);
       case State::INFO_HEADER_REST: return ReadInfoHeaderRest(aData, aLength);
       case State::BITFIELDS:        return ReadBitfields(aData, aLength);
@@ -491,14 +505,6 @@ nsBMPDecoder::ReadFileHeader(const char* aData, size_t aLength)
   return Transition::To(State::INFO_HEADER_SIZE, BIHSIZE_FIELD_LENGTH);
 }
 
-LexerTransition<nsBMPDecoder::State>
-nsBMPDecoder::ReadClipboardHeader(const char* aData, size_t aLength)
-{
-  // With the clipboard, the data offset is the header length.
-  mH.mDataOffset = LittleEndian::readUint32(aData);
-  return ReadInfoHeaderSize(aData, aLength);
-}
-
 // We read the info header in two steps: (a) read the mBIHSize field to
 // determine how long the header is; (b) read the rest of the header.
 LexerTransition<nsBMPDecoder::State>
@@ -609,6 +615,13 @@ nsBMPDecoder::ReadInfoHeaderRest(const char* aData, size_t aLength)
       // Bitfields are present in the info header, so we can read them
       // immediately.
       mBitFields.ReadFromHeader(aData + 36, /* aReadAlpha = */ true);
+
+      // If this came from the clipboard, then we know that even if the header
+      // explicitly includes the bitfield masks, we need to add an additional
+      // offset for the start of the RGB data.
+      if (mIsForClipboard) {
+        mH.mDataOffset += BitFields::LENGTH;
+      }
     } else {
       // Bitfields are present after the info header, so we will read them in
       // ReadBitfields().
@@ -723,6 +736,12 @@ nsBMPDecoder::ReadColorTable(const char* aData, size_t aLength)
     aData += mBytesPerColor;
   }
 
+  // If we are decoding a BMP from the clipboard, we did not know the data
+  // offset in advance. It is defined as just after the header and color table.
+  if (mIsForClipboard) {
+    mH.mDataOffset += mPreGapLength;
+  }
+
   // We know how many bytes we've read so far (mPreGapLength) and we know the
   // offset of the pixel data (mH.mDataOffset), so we can determine the length
   // of the gap (possibly zero) between the color table and the pixel data.
diff --git a/image/decoders/nsBMPDecoder.h b/image/decoders/nsBMPDecoder.h
index 258381649..4b9568487 100644
--- a/image/decoders/nsBMPDecoder.h
+++ b/image/decoders/nsBMPDecoder.h
@@ -152,7 +152,6 @@ private:
 
   enum class State {
     FILE_HEADER,
-    CLIPBOARD_HEADER,
     INFO_HEADER_SIZE,
     INFO_HEADER_REST,
     BITFIELDS,
@@ -172,7 +171,7 @@ private:
   nsBMPDecoder(RasterImage* aImage, uint32_t aDataOffset);
 
   // Helper constructor called by the other two.
-  nsBMPDecoder(RasterImage* aImage, State aState, size_t aLength);
+  nsBMPDecoder(RasterImage* aImage, State aState, size_t aLength, bool aForClipboard);
 
   int32_t AbsoluteHeight() const { return abs(mH.mHeight); }
 
@@ -181,7 +180,6 @@ private:
   void FinishRow();
 
   LexerTransition<State> ReadFileHeader(const char* aData, size_t aLength);
-  LexerTransition<State> ReadClipboardHeader(const char* aData, size_t aLength);
   LexerTransition<State> ReadInfoHeaderSize(const char* aData, size_t aLength);
   LexerTransition<State> ReadInfoHeaderRest(const char* aData, size_t aLength);
   LexerTransition<State> ReadBitfields(const char* aData, size_t aLength);
@@ -200,6 +198,9 @@ private:
   // If the BMP is within an ICO file our treatment of it differs slightly.
   bool mIsWithinICO;
 
+  // If the BMP is decoded from the clipboard, we start with a data offset.
+  bool mIsForClipboard;
+
   bmp::BitFields mBitFields;
 
   // Might the image have transparency? Determined from the headers during