fix: SIGSEGV on parsing of config file.

[hassec]

This reverts a bug that was introduced by the clang-tidy change in #1659. (fyi @neheb)

I'm just opening this already to make people aware of it.
Before merging, I'd like to add a test that uses a config file, as it's a bit of a shame we hadn't noticed this.

[codecov]

Codecov Report

Merging #1783 (92f7d86) into main (8f126c0) will increase coverage by 0.73%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1783      +/-   ##
==========================================
+ Coverage   67.48%   68.22%   +0.73%     
==========================================
  Files         151      151              
  Lines       20805    20848      +43     
==========================================
+ Hits        14041    14223     +182     
+ Misses       6764     6625     -139     

Impacted Files	Coverage Δ
src/ini.cpp	70.29% <100.00%> (+63.56%)	⬆️
src/makernote_int.cpp	80.44% <100.00%> (+1.21%)	⬆️
src/sonymn_int.cpp	73.21% <0.00%> (-15.68%)	⬇️
src/iptc.cpp	75.72% <0.00%> (-0.34%)	⬇️
src/version.cpp	91.51% <0.00%> (-0.21%)	⬇️
src/exiv2app.hpp	100.00% <0.00%> (ø)
src/tags_int.cpp	87.20% <0.00%> (ø)
src/tags_int.hpp	90.90% <0.00%> (ø)
samples/exifprint.cpp	72.22% <0.00%> (ø)
src/tiffimage_int.cpp	89.28% <0.00%> (ø)
... and 19 more

---

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 8f126c0...92f7d86. Read the comment docs.

[neheb]

Makes sense.

[kevinbackhouse]

Interesting. I guess this is because INIReader::ValueHandler tries to add elements to reader->_values before construction is complete. I don't much like that void *user stuff!

[hassec]

@kevinbackhouse yep that's it.
I don't like it either but it's an external library that is designed with C in mind and the C++ part really is just a thin wrapper around it.

But on top of that, I'm not really a fan of having this library just dropped into our source like this and then even applying clang-tidy to it.
We need a cleaner way to handle 3rd party libs, so I added it to #1515

One could consider replacing it with something else but let's leave that discussion to #1515

[kevinbackhouse]

@hassec: Oh, I see. All of this code, including that INIReader::ValueHandler stuff, is copied from https://github.com/benhoyt/inih.
It looks like it's available as a library on Ubuntu, but I don't know what the situation is on other platforms.

[kevinbackhouse]

I think I would prefer if the test didn't write a file into the user's home directory. One suggestion is to get getExiv2ConfigPath to try getenv("HOME") first, so that the test can use the environment to tell it where to find the config file. Another suggestion is to a new command-line argument, like --config=....

[hassec]

I kind of understand that point. But the test is set up in a way such that it can't overwrite a preexisting file, and it removes the file it creates at the end.
So there really isn't an observable impact for the user.

--config is an option I actually also thought about, but decided against it because I didn't want to add an extra option just to make the testing easier.
The config file itself is already a bit of a corner case feature that is only used in a few maker notes for lens name overwriting.

if you have strong feelings against the current solution, I'd say the --config flag option is the best one if we want to gain test coverage over this feature.

[kevinbackhouse]

The main reason why I don't like it is that it could make Exiv2 look like a malicious repo. People are quite worried about supply-chain attacks these days, so a build or test script that modifies files outside its own directory could raise suspicions.

[hassec]

@kevinbackhouse fair enough! Let me have a look at adding a --config flag.
Haven't looked at the options parser in exiv2 yet, but I'd hope it should be easy enough 😅

[kevinbackhouse]

Another option that might be relatively quick to implement would be to check the current directory for a file named .exiv2 first, before looking in the home directory.

[hassec]

@kevinbackhouse I've implemented the current working directory version because it seemed more straight forward.

Turns out adding the option to the exiv2 app isn't hard but then one has to think about how to properly pass those forward to the library part of exiv2, and then also expose this setting to the library users. That would have been more work so I went with your suggestion 😉

Also, thanks for reviewing!! 👍