Add some XML validation to avoid xmpsdk bugs (backport #1878)

[mergify]

This is an automatic backport of pull request #1878 done by Mergify.
Cherry-pick of b35cc5f has failed:

On branch mergify/bp/0.27-maintenance/pr-1878
Your branch is up to date with 'origin/0.27-maintenance'.

You are currently cherry-picking commit b35cc5ff.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   src/CMakeLists.txt
	modified:   src/xmp.cpp
	modified:   tests/bugfixes/github/test_CVE_2017_14857.py
	modified:   tests/bugfixes/github/test_CVE_2017_14858.py
	modified:   tests/bugfixes/github/test_issue_1713.py
	modified:   tests/bugfixes/github/test_issue_1819.py
	modified:   tests/bugfixes/github/test_issue_428.py
	modified:   tests/bugfixes/github/test_issue_851.py
	modified:   tests/bugfixes/github/test_issue_ghsa_8949_hhfh_j7rj.py
	modified:   tests/bugfixes/github/test_issue_ghsa_v5g7_46xf_h728.py

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	deleted by us:   tests/bugfixes/github/test_coverage_xmpsidecar_isXmpType.py

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

---

Mergify commands and options

More conditions and actions can be found in the documentation.

You can also trigger Mergify actions by commenting on this pull request:

• @Mergifyio refresh will re-evaluate the rules
• @Mergifyio rebase will rebase this PR on its base branch
• @Mergifyio update will merge the base branch into this PR
• @Mergifyio backport <destination> will backport this PR on <destination> branch

Additionally, on Mergify dashboard you can:

• look at your merge queues
• generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.io/

[codecov]

Codecov Report

Merging #1880 (96c2fa8) into 0.27-maintenance (962ef36) will increase coverage by 0.06%.
The diff coverage is 73.68%.

@@                 Coverage Diff                  @@
##           0.27-maintenance    #1880      +/-   ##
====================================================
+ Coverage             46.21%   46.27%   +0.06%     
====================================================
  Files                   146      146              
  Lines                 22874    22950      +76     
  Branches              11762    11780      +18     
====================================================
+ Hits                  10572    10621      +49     
- Misses                 6696     6713      +17     
- Partials               5606     5616      +10     

Impacted Files	Coverage Δ
src/xmp.cpp	59.67% <73.68%> (+2.05%)	⬆️
src/iptc.cpp	49.23% <0.00%> (-0.39%)	⬇️
src/convert.cpp	27.45% <0.00%> (-0.34%)	⬇️
src/value.cpp	60.32% <0.00%> (-0.15%)	⬇️
src/actions.cpp	62.48% <0.00%> (-0.08%)	⬇️
src/xmpsidecar.cpp	30.90% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 962ef36...96c2fa8. Read the comment docs.

Pull request has been modified.

[kevinbackhouse]

I'm still fighting with cmake. Can't get it to link with libexpat on all platforms.

Pull request has been modified.

[clanmills]

@kevinbackhouse I use expat directly in samples/geotag.cpp. The CMake code to compile and link geotag is in samples/CMakeLists.txt

if( EXPAT_FOUND )
    add_executable(        geotag    geotag.cpp)
    list(APPEND APPLICATIONS geotag)
    target_link_libraries(geotag 
        PRIVATE
            exiv2-xmp
            ${EXPAT_LIBRARIES}
    )
    target_include_directories(geotag PRIVATE ${CMAKE_BINARY_DIR})          # exv_conf.h 
    target_include_directories(geotag PRIVATE ${CMAKE_SOURCE_DIR}/include)  # <exiv2/exiv2.hpp>    
    target_include_directories(geotag PRIVATE ${EXPAT_INCLUDE_DIR})
    target_include_directories(geotag PRIVATE ${CMAKE_SOURCE_DIR}/src) # To find unused.h

    if (WIN32)
        target_compile_definitions(geotag PRIVATE XML_STATIC)
    endif()

    if (MSVC)
        set_target_properties(geotag PROPERTIES LINK_FLAGS "/ignore:4099")
    endif()

    install( TARGETS       geotag    RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR})
endif()

If you only use the new class XMLvalidator in the exiv2 command-line program, you'll need to link expat in much the same way that I do for samples/geotag.

If you use your new class in the Exiv2 library, you shouldn't need to link expat into every application. If an application isn't calling this explicitly, it should like as any other internal library code. However the library will probably need something similar to the geotag/CMake code to link and use expat.

If you use your new class in the XMPsdk (which is what I thought you had done), you shouldn't need anything to link expat as the XMPsdk already links and uses expat.

[kevinbackhouse]

It looks this bit was the solution:

if (MSVC)
    target_compile_definitions(exiv2lib PRIVATE XML_STATIC)
endif()

I have no idea what that does!

[kevinbackhouse]

@clanmills: regarding your comment about not linking expat when it isn't needed: I think that's something that I need to fix. Right now, I am linking with expat unconditionally. I see now that a lot of the code in xmp.cpp is enclosed in #ifdef EXV_HAVE_XMP_TOOLKIT. I think I need to do that for XMLValidator too. Then I'll only need to link with expat when XMP is enabled.

[clanmills]

I think it says "only real MSVC wizards can understand this shit". I just can't remember. I think it's probably better to use the same condition used for geotag.cpp:

    if (WIN32)
        target_compile_definitions(geotag PRIVATE XML_STATIC)
    endif()

Good team work got us here.

[clanmills]

I totally agree. XMLvalidator should only be compiled and linked when XMPsdk is in the mix. It has no other purpose.

It's possible for geotag to be used without XMPsdk. I didn't try to solve that and nobody has ever requested it. My attitude is:

1. You want to build samples/geotag with CMake, you need XMPsdk buddy.
2. You want to build samples/geotag without XMPsdk, see the comment in samples/geotag.cpp:

// geotag.cpp
// Sample program to read gpx files and update images with GPS tags
// g++ geotag.cpp -o geotag -lexiv2 -lexpat

[clanmills]

Yup. @kevinbackhouse can you make two changes and then we're done:

1. Don't compile XMLvalidator unless XMPsdk is in the mix.
2. Use if ( WIN32) to be consistent with the geotag code.

When that passes the CI, I'll approve the change and we're done. And much the same with the isize puzzle. Submit something like my patch and we're done (until the next miserable mystery).

The sun is just starting to come out in Camberley. Forecast is great from 17:00 onwards and good for Saturday and Sunday.