Fix issues detected with PVS-Studio + other little improvements (#1689)

* avoid re-declaration of constant variables

* Replace pthreads critical section with std::mutex

* ci - better naming

* cmake - increase minimum version to 3.11. Use project DESCRIPTION

* fix - do not treat string::find() return type as bool

* remove conditions that were always true

* remove condition that were always false

* Remove EXV_HAVE_GMTIME_R which is not used anymore

* pixelWidth_ was inherited from Exiv2::Image

The width & height variables in the TiffImage class need to be mutable
to be able to change their values on the getters pixelHeight() and
pixelWidth() ... Do not ask me why ...

* Remove superfluous if

* pvs:V766 item with identical key added already

* pvs:V730 not all members were initialized (time)

* pvs:V730 not all members are initialized

* pvs:v668 no point in testing pointer against null after new

* pvs:V1048 variable assigned the same value

* replace c-style dynamic vector with std one

* pvs:547 fakeData is always true

* Remove useless constructor in derived class

* pvs:V690 modern way to disable copy-ctor

* Replace malloc/free with new/delete. No need to check for null

* pvs:V1028 cast operands and not result

* Remove custom MIN/MAX functions

* pvs:V595 pointer used before verified against null

* pvs: index used before being checked

* pvs:V1028 possible overflow. Cast operands

* pvs:v575 potential null pointer passed to other functions

* pvs:V547 deal with always true/false expressions

* pvs:V560 part of conditional expressions always false or true

* pvs:V701 possible break in realloc -> move to std::vector

* Make some classes 'final'

* Replace sprintf with std::to_string()

* fix compilation on windows

.github/workflows/on_push_BasicWinLinMac.yml

@@ -49,7 +49,7 @@ jobs:
           cmake --build .
 
 
-      - name: Unit Test
+      - name: Test
         run: |
           cd build
           ctest --output-on-failure
@@ -80,7 +80,7 @@ jobs:
           cmake -GNinja -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_ENABLE_CURL=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DEXIV2_ENABLE_BMFF=ON -DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON -DCMAKE_INSTALL_PREFIX=install -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON ..
           cmake --build .
 
-      - name: Unit Test
+      - name: Test
         run: |
           cd build
           ctest --output-on-failure
@@ -112,7 +112,7 @@ jobs:
           cmake -GNinja -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_ENABLE_CURL=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DEXIV2_ENABLE_BMFF=ON -DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON -DCMAKE_INSTALL_PREFIX=install -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON -DCMAKE_CXX_FLAGS="-Wno-deprecated-declarations" ..
           cmake --build .
 
-      - name: Unit Test
+      - name: Test
         run: |
           cd build
           ctest --output-on-failure

CMakeLists.txt

@@ -1,4 +1,5 @@
-cmake_minimum_required( VERSION 3.5.0 )
+# Minimum version imposed by Centos:8
+cmake_minimum_required( VERSION 3.11.0 ) 
 
 project(exiv2          # use TWEAK to categorize the build
     VERSION 1.00.0.9   # 1.00.0    = GM               (tagged and released)
@@ -7,6 +8,7 @@ project(exiv2          # use TWEAK to categorize the build
                        # 1.00.0.2  = RC2              (tagged and released)
                        # 1.00.0.20 = RC2 Preview
                        # 1.00.0.29 = RC2 Development
+    DESCRIPTION    "Exif/IPTC/Xmp C++ metadata library and tools plus ICC Profiles, Previews and more."
     LANGUAGES CXX
 )
 
@@ -54,9 +56,7 @@ option( BUILD_WITH_CCACHE             "Use ccache to speed up compilations"
 option( BUILD_WITH_COVERAGE           "Add compiler flags to generate coverage stats"         OFF )
 include(cmake/gcovr.cmake REQUIRED)
 
-set( PACKAGE_BUGREPORT                "http://github.com/exiv2/exiv2" )
 set( PACKAGE_URL                      "https://exiv2.org")
-set( PROJECT_DESCRIPTION              "Exif/IPTC/Xmp C++ metadata library and tools plus ICC Profiles, Previews and more.")
 
 if ( EXIV2_ENABLE_EXTERNAL_XMP )
     set(EXIV2_ENABLE_XMP OFF)

cmake/findDependencies.cmake

@@ -18,9 +18,6 @@ if (APPLE)
      set(CMAKE_FIND_FRAMEWORK NEVER)
 endif()
 
-
-find_package(Threads REQUIRED)
-
 if( EXIV2_ENABLE_PNG )
     find_package( ZLIB REQUIRED )
 endif( )

cmake/generateConfigFile.cmake

@@ -23,7 +23,6 @@ set(EXV_HAVE_ICONV       ${ICONV_FOUND})
 set(EXV_HAVE_LIBZ        ${ZLIB_FOUND})
 set(EXV_UNICODE_PATH     ${EXIV2_ENABLE_WIN_UNICODE})
 
-check_cxx_symbol_exists(gmtime_r    time.h         EXV_HAVE_GMTIME_R)
 check_cxx_symbol_exists(mmap        sys/mman.h     EXV_HAVE_MMAP )
 check_cxx_symbol_exists(munmap      sys/mman.h     EXV_HAVE_MUNMAP )
 check_cxx_symbol_exists(strerror_r  string.h       EXV_HAVE_STRERROR_R )

contrib/vs2019/solution/exv_conf.h

@@ -12,9 +12,6 @@
 // Define if you require PNG support.
 #define EXIV2_ENABLE_PNG
 
-// Define if you have the `gmtime_r' function.
-/* #undef EXV_HAVE_GMTIME_R */
-
 // Define if you have the <libintl.h> header file.
 /* #undef EXV_HAVE_LIBINTL_H */
 

include/exiv2/basicio.hpp

@@ -997,16 +997,11 @@ namespace Exiv2 {
        //@}
 
     protected:
-        //! @name Creators
-        //@{
-        //! Default Constructor
-        RemoteIo() {p_=NULL;}
-        //@}
 
         // Pimpl idiom
         class Impl;
         //! Pointer to implementation
-        Impl* p_;
+        Impl* p_ {nullptr};
     }; // class RemoteIo
 
     /*!
@@ -1043,12 +1038,6 @@ namespace Exiv2 {
         HttpIo& operator=(const HttpIo& rhs);
         // Pimpl idiom
         class HttpImpl;
-
-        //! @name Creators
-        //@{
-        //! Default Destructor
-        virtual ~HttpIo(){}
-        //@}
     };
 
 #ifdef EXV_USE_CURL
@@ -1098,12 +1087,6 @@ namespace Exiv2 {
         CurlIo& operator=(const CurlIo& rhs);
         // Pimpl idiom
         class CurlImpl;
-
-        //! @name Creators
-        //@{
-        //! Default Destructor
-        virtual ~CurlIo(){}
-        //@}
     };
 #endif
 

include/exiv2/bmffimage.hpp

@@ -120,7 +120,7 @@ namespace Exiv2
         int pixelHeight() const;
         //@}
 
-        Exiv2::ByteOrder endian_ ;
+        Exiv2::ByteOrder endian_{Exiv2::bigEndian};
 
     private:
         void openOrThrow();
@@ -136,14 +136,14 @@ namespace Exiv2
             return std::string(2*i,' ');
         }
 
-        uint32_t                 fileType_;
+        uint32_t                 fileType_{0};
         std::set<uint64_t>       visits_;
-        uint64_t                 visits_max_;
-        uint16_t                 unknownID_;  // 0xffff
-        uint16_t                 exifID_;
-        uint16_t                 xmpID_;
+        uint64_t                 visits_max_{0};
+        uint16_t                 unknownID_{0xffff};
+        uint16_t                 exifID_{0xffff};
+        uint16_t                 xmpID_{0};
         std::map<uint32_t, Iloc> ilocs_;
-        bool                     bReadMetadata_;
+        bool                     bReadMetadata_{false};
         //@}
 
         /*!

include/exiv2/tiffimage.hpp

@@ -117,8 +117,8 @@ namespace Exiv2 {
         // DATA
         mutable std::string primaryGroup_;     //!< The primary group
         mutable std::string mimeType_;         //!< The MIME type
-        mutable int pixelWidth_;               //!< Width of the primary image in pixels
-        mutable int pixelHeight_;              //!< Height of the primary image in pixels
+        mutable int pixelWidthPrimary_;        //!< Width of the primary image in pixels
+        mutable int pixelHeightPrimary_;       //!< Height of the primary image in pixels
 
     }; // class TiffImage
 

include/exiv2/types.hpp

@@ -65,12 +65,6 @@
  */
 #define EXV_CALL_MEMBER_FN(object,ptrToMember)  ((object).*(ptrToMember))
 
-// Simple min and max macros
-//! Simple common min macro
-#define EXV_MIN(a,b) ((a) < (b) ? (a) : (b))
-//! Simple common max macro
-#define EXV_MAX(a,b) ((a) > (b) ? (a) : (b))
-
 #if defined(__GNUC__) && (__GNUC__ >= 4) || defined(__clang__)
 #define EXV_WARN_UNUSED_RESULT __attribute__ ((warn_unused_result))
 #elif defined(_MSC_VER) && (_MSC_VER >= 1700)

samples/geotag.cpp

@@ -289,36 +289,26 @@ time_t Position::deltaMax_ = 60 ;
 
 ///////////////////////////////////////////////////////////
 // UserData - used by XML Parser
-class UserData
+struct UserData final
 {
-public:
-    explicit UserData(Options& options):
-        indent(0)
-      , count(0)
-      , nTrkpt(0)
-      , bTime(false)
-      , bEle(false)
-      , ele(0.0)
-      , lat(0.0)
-      , lon(0.0)
-      , options_(options)
+    explicit UserData(Options& options)
+        : options_(options)
     {}
-    virtual ~UserData() = default;
 
     //  public data members
-    int         indent;
-    size_t      count ;
+    int         indent{0};
+    size_t      count{0};
     Position    now ;
     Position    prev;
-    int         nTrkpt;
-    bool        bTime ;
-    bool        bEle  ;
-    double      ele;
-    double      lat;
-    double      lon;
+    int         nTrkpt{0};
+    bool        bTime{false};
+    bool        bEle{false};
+    double      ele{0.};
+    double      lat{0.};
+    double      lon{0.};
     std::string xmlt;
     std::string exift;
-    time_t      time;
+    time_t      time{0};
     Options&    options_;
 // static public data memembers
 };
@@ -669,8 +659,8 @@ int readFile(const char* path, const Options& /* options */)
             if ( sina(ext,code) )
                 nResult = typeCode;
         }
+        fclose(f);
     }
-    if ( f ) fclose(f) ;
 
     return nResult ;
 }
@@ -809,7 +799,7 @@ int main(int argc,const char* argv[])
     shorts["-D"] = "-delta";
     shorts["-s"] = "-delta";
     shorts["-X"] = "-dryrun";
-    shorts["-a"] = "-ascii";
+    shorts["-A"] = "-ascii";
 
     Options options ;
     options.help    = sina(keywords[kwHELP   ],argv) || argc < 2;

src/CMakeLists.txt

@@ -215,7 +215,6 @@ if (NOT MSVC)
         target_link_libraries( exiv2lib PRIVATE psapi ws2_32 shell32 )
     endif()
 
-    target_link_libraries( exiv2lib PRIVATE Threads::Threads)
 else()
     target_link_libraries( exiv2lib PRIVATE psapi ws2_32 shell32 )
 endif()

src/actions.cpp

@@ -50,6 +50,7 @@
 #include <ctime>
 #include <cmath>
 #include <cassert>
+#include <mutex>
 #include <stdexcept>
 #include <sys/types.h>                  // for stat()
 #include <sys/stat.h>                   // for stat()
@@ -71,6 +72,7 @@
 // *****************************************************************************
 // local declarations
 namespace {
+    std::mutex cs;
 
     //! Helper class to set the timestamp of a file to that of another file
     class Timestamp {
@@ -253,7 +255,6 @@ namespace Action {
         try {
             path_ = path;
             int rc = 0;
-            Exiv2::PrintStructureOption option = Exiv2::kpsNone ;
             switch (Params::instance().printMode_) {
                 case Params::pmSummary:   rc = Params::instance().greps_.empty() ? printSummary() : printList(); break;
                 case Params::pmList:      rc = printList();        break;
@@ -262,14 +263,10 @@ namespace Action {
                 case Params::pmStructure: rc = printStructure(std::cout,Exiv2::kpsBasic, path_)     ; break;
                 case Params::pmRecursive: rc = printStructure(std::cout,Exiv2::kpsRecursive, path_) ; break;
                 case Params::pmXMP:
-                    if (option == Exiv2::kpsNone)
-                        option = Exiv2::kpsXMP;
-                    rc = setModeAndPrintStructure(option, path_,binary());
+                    rc = setModeAndPrintStructure(Exiv2::kpsXMP, path_,binary());
                     break;
                 case Params::pmIccProfile:
-                    if (option == Exiv2::kpsNone)
-                        option = Exiv2::kpsIccProfile;
-                    rc = setModeAndPrintStructure(option, path_,binary());
+                    rc = setModeAndPrintStructure(Exiv2::kpsIccProfile, path_,binary());
                     break;
             }
             return rc;
@@ -540,8 +537,6 @@ namespace Action {
 
         bool first = true;
         if (Params::instance().printItems_ & Params::prTag) {
-            if (!first)
-                std::cout << " ";
             first = false;
             std::cout << "0x" << std::setw(4) << std::setfill('0') << std::right << std::hex << md.tag();
         }
@@ -1854,34 +1849,12 @@ namespace {
         return os.str();
     } // tm2Str
 
-// use static CS/MUTEX to make temporaryPath() thread safe
-#if defined(_MSC_VER) || defined(__MINGW__)
- static CRITICAL_SECTION cs;
-#else
- /* Unix/Linux/Cygwin/macOS */
- #include <pthread.h>
- /* This is the critical section object (statically allocated). */
- #if defined(__APPLE__)
-  #if defined(PTHREAD_RECURSIVE_MUTEX_INITIALIZER)
-    static pthread_mutex_t cs =  PTHREAD_RECURSIVE_MUTEX_INITIALIZER;
-   #else
-    static pthread_mutex_t cs =  PTHREAD_MUTEX_INITIALIZER;
-  #endif
- #else
-  #if defined(PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP)
-    pthread_mutex_t cs = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
-#else
-    pthread_mutex_t cs = PTHREAD_MUTEX_INITIALIZER;
-#endif
-#endif
-#endif
-
  std::string temporaryPath()
  {
      static int count = 0;
+     std::lock_guard<std::mutex> guard(cs);
 
 #if defined(_MSC_VER) || defined(__MINGW__)
-        EnterCriticalSection(&cs);
         char lpTempPathBuffer[MAX_PATH];
         GetTempPath(MAX_PATH,lpTempPathBuffer);
         std::string tmp(lpTempPathBuffer);
@@ -1890,22 +1863,14 @@ namespace {
         DWORD  pid = ::GetProcessId(process);
 #else
         pid_t  pid = ::getpid();
-        pthread_mutex_lock( &cs );
         std::string tmp = "/tmp/";
 #endif
-        char sCount[13];
-        sprintf(sCount,"_%d",++count); /// \todo replace by std::snprintf on master
-
-        std::string result = tmp + Exiv2::toString(pid) + sCount ;
-        if ( Exiv2::fileExists(result) ) std::remove(result.c_str());
-
-#if defined(_MSC_VER) || defined(__MINGW__)
-        LeaveCriticalSection(&cs);
-#else
-        pthread_mutex_unlock( &cs );
-#endif
+    std::string result = tmp + Exiv2::toString(pid) + "_" + std::to_string(count);
+    if (Exiv2::fileExists(result)) {
+        std::remove(result.c_str());
+    }
 
-        return result;
+    return result;
  }
 
     int metacopy(const std::string& source,

src/actions.hpp

@@ -112,6 +112,10 @@ namespace Action {
           this method.
         */
         static TaskFactory& instance();
+
+        //! Prevent copy construction: not implemented.
+        TaskFactory(const TaskFactory& rhs) = delete;
+
         //! Destructor
         void cleanup();
 
@@ -144,8 +148,6 @@ namespace Action {
     private:
         //! Prevent construction other than through instance().
         TaskFactory();
-        //! Prevent copy construction: not implemented.
-        TaskFactory(const TaskFactory& rhs);
 
         //! Pointer to the one and only instance of this class.
         static TaskFactory* instance_;