Remove 2nd param from Photoshop::isIrb() since it is always hardcoded

Author: piponazo

include/exiv2/jpgimage.hpp

@@ -27,11 +27,11 @@ struct EXIV2API Photoshop {
   static constexpr uint16_t preview_ = 0x040c;                         //!< %Photoshop preview marker
 
   /// @brief Checks an IRB
-  /// @param pPsData        Existing IRB buffer
-  /// @param sizePsData     Size of the IRB buffer
-  /// @return true  if the IRB marker is known and the buffer is big enough to check this;<BR>
-  ///   false otherwise
-  static bool isIrb(const byte* pPsData, size_t sizePsData);
+  /// @param pPsData  Existing IRB buffer. It is expected to be of size 4.
+  /// @return true  if the IRB marker is known
+  /// @todo This should be an implementation detail and not exposed in the API. An attacker could try to pass
+  ///   a smaller buffer or null pointer.
+  static bool isIrb(const byte* pPsData);
 
   /// @brief Validates all IRBs
   /// @param pPsData        Existing IRB buffer

src/jpgimage.cpp

@@ -35,9 +35,11 @@ static inline bool inRange2(int value, int lo1, int hi1, int lo2, int hi2) {
   return inRange(lo1, value, hi1) || inRange(lo2, value, hi2);
 }
 
-bool Photoshop::isIrb(const byte* pPsData, size_t sizePsData) {
-  if (sizePsData < 4)
+bool Photoshop::isIrb(const byte* pPsData) {
+  if (pPsData == nullptr) {
     return false;
+  }
+  /// \todo check if direct array comparison is faster than a call to memcmp
   return std::any_of(irbId_.begin(), irbId_.end(), [pPsData](auto id) { return memcmp(pPsData, id, 4) == 0; });
 }
 
@@ -69,7 +71,7 @@ int Photoshop::locateIrb(const byte* pPsData, size_t sizePsData, uint16_t psTag,
   std::cerr << "Photoshop::locateIrb: ";
 #endif
   // Data should follow Photoshop format, if not exit
-  while (position <= sizePsData - 12 && isIrb(pPsData + position, 4)) {
+  while (position <= sizePsData - 12 && isIrb(pPsData + position)) {
     const byte* hrd = pPsData + position;
     position += 4;
     uint16_t type = getUShort(pPsData + position, bigEndian);

src/psdimage.cpp

@@ -175,7 +175,7 @@ void PsdImage::readMetadata() {
       throw Error(ErrorCode::kerNotAnImage, "Photoshop");
     }
 
-    if (!Photoshop::isIrb(buf, 4)) {
+    if (!Photoshop::isIrb(buf)) {
       break;  // bad resource type
     }
     uint16_t resourceId = getUShort(buf + 4, bigEndian);
@@ -413,7 +413,7 @@ void PsdImage::doWriteMetadata(BasicIo& outIo) {
     // read resource type and ID
     uint32_t resourceType = getULong(buf, bigEndian);
 
-    if (!Photoshop::isIrb(buf, 4)) {
+    if (!Photoshop::isIrb(buf)) {
       throw Error(ErrorCode::kerNotAnImage, "Photoshop");  // bad resource type
     }
     uint16_t resourceId = getUShort(buf + 4, bigEndian);

unitTests/test_Photoshop.cpp

@@ -12,20 +12,23 @@ constexpr std::array validMarkers{"8BIM", "AgHg", "DCSR", "PHUT"};
 
 TEST(Photoshop_isIrb, returnsTrueWithValidMarkers) {
   for (const auto& marker : validMarkers) {
-    ASSERT_TRUE(Photoshop::isIrb(reinterpret_cast<const byte*>(marker), 4));
+    ASSERT_TRUE(Photoshop::isIrb(reinterpret_cast<const byte*>(marker)));
   }
 }
 
 
 TEST(Photoshop_isIrb, returnsFalseWithInvalidMarkers) {
-  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("7BIM"), 4));
-  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("AGHg"), 4));
-  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("dcsr"), 4));
-  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("LUIS"), 4));
+  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("7BIM")));
+  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("AGHg")));
+  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("dcsr")));
+  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("LUIS")));
 }
 
-TEST(Photoshop_isIrb, returnsFalseWithInvalidSize) {
-  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("8BIM"), 3));
-  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("8BIM"), 0));
-  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("8BIM"), 5));
+TEST(Photoshop_isIrb, returnsFalseWithNullPointer) {
+  ASSERT_FALSE(Photoshop::isIrb(nullptr));
+}
+
+/// \note probably this is not safe and we need to remove it
+TEST(Photoshop_isIrb, returnsFalseWithShorterMarker) {
+  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("8BI")));
 }