Add a workflow build/test for macOS+Sanitizers #2123
Conversation
|
It looks like the new build is being picked up and run: https://github.com/Exiv2/exiv2/runs/5413848387?check_suite_focus=true 👍 |
Codecov Report
@@ Coverage Diff @@
## main #2123 +/- ##
==========================================
- Coverage 63.98% 63.38% -0.61%
==========================================
Files 97 97
Lines 19084 19189 +105
Branches 9750 9715 -35
==========================================
- Hits 12211 12163 -48
- Misses 4599 4758 +159
+ Partials 2274 2268 -6
Continue to review full report at Codecov.
|
|
Great, now we can see the test which is failing in CI: It should be possible to check that test and run the exact command from a debugger to discover what is happening there. |
|
I'll look at the x86_64 build and update this report. The arm64/ASAN build is a disaster zone. We should try to simplify that and report it on Apple Radar. I believe @piponazo changed Exiv2::ExifData to Exiv2::Metadata and the arm64/ASAN build is throwing |
|
I don't have the full stack trace in front of me, but I did step through the debugger. The problem in this test is in the code that extracts the ISO speed from the metadata. IIRC without the sanitizer the code that converts a float to an int64_t converts +Inf to the minimum int64_t value. The code that calls it is looking for a value > 0, so without the sanitizer the logic works. |
|
parseInt64() is decoding string template<typename T>
T stringTo(const std::string& s, bool& ok)
{
std::istringstream is(s);
T tmp = T();
ok = bool(is >> tmp);
std::string rest;
is >> std::skipws >> rest;
if (!rest.empty()) ok = false; <-- not setting ok = false
return tmp;
}
int64_t parseInt64(const std::string& s, bool& ok)
{
int64_t ret = stringTo<int64_t>(s, ok);
if (ok) return ret;
auto f = stringTo<float>(s, ok);
if (ok) return static_cast<int64_t>(f); <--- explodes
Rational r = stringTo<Rational>(s, ok);
if (ok) {
if (r.second <= 0) {
ok = false;
return 0;
}
return static_cast<int64_t>(static_cast<float>(r.first) / r.second);
}
}The quick fix is as follows and survives a "full test" (unit_tests, samples, everything) diff --git a/include/exiv2/types.hpp b/include/exiv2/types.hpp
index ed3fac48..3b0a1844 100644
--- a/include/exiv2/types.hpp
+++ b/include/exiv2/types.hpp
@@ -501,7 +501,7 @@ namespace Exiv2 {
ok = bool(is >> tmp);
std::string rest;
is >> std::skipws >> rest;
- if (!rest.empty()) ok = false;
+ if (!rest.empty() || s.find("inf") != std::string::npos) ok = false;
return tmp;
}
I'm investigating the source of 675 rmills@rmillsmm-local:~/gnu/github/exiv2/main/xcode_build $ exiv2 -g iso/i ../test/data/issue_1827_poc.crw
Exif.CanonSi.AutoISO Short 1 2.00187e+06
Exif.CanonSi.ISOSpeed Short 1 inf
676 rmills@rmillsmm-local:~/gnu/github/exiv2/main/xcode_build $ exiv2 -pv -g iso/i ../test/data/issue_1827_poc.crw
0x0001 CanonSi AutoISO Short 1 14632
0x0002 CanonSi ISOSpeed Short 1 57446
677 rmills@rmillsmm-local:~/gnu/github/exiv2/main/xcode_build $Every tag in Exif.CanonSi looks wrong and I get the same suspicious output on branch 0.27-maintenance. 695 rmills@rmillsmm-local:~/gnu/github/exiv2/main/build $ env DYLD_LIBRARY_PATH=$PWD/lib bin/exiv2 -g CanonSi ../test/data/issue_1827_poc.crw
Exif.CanonSi.AutoISO Short 1 2.00187e+06
Exif.CanonSi.ISOSpeed Short 1 inf
Exif.CanonSi.MeasuredEV Short 1 749.31
Exif.CanonSi.TargetAperture Short 1 57649
Exif.CanonSi.TargetShutterSpeed Short 1 1 s
Exif.CanonSi.ExposureCompensation Short 1 147
Exif.CanonSi.WhiteBalance Short 1 (21327)
Exif.CanonSi.SlowShutter Short 1 (54101)
Exif.CanonSi.Sequence Short 1 28624
Exif.CanonSi.OpticalZoomCode Short 1 29367
Exif.CanonSi.CameraTemperature Short 1 19286 °C
Exif.CanonSi.FlashGuideNumber Short 1 734
Exif.CanonSi.AFPointUsed Short 1 2 focus points; used
Exif.CanonSi.FlashBias Short 1 (63793)
Exif.CanonSi.AutoExposureBracketing Short 1 (37805)
Exif.CanonSi.AEBBracketValue Short 1 44174
Exif.CanonSi.ControlMode Short 1 20743
Exif.CanonSi.SubjectDistance Short 1 230.21 m
Exif.CanonSi.ApertureValue Short 1 55444
Exif.CanonSi.ShutterSpeedValue Short 1 1 s
Exif.CanonSi.MeasuredEV2 Short 1 7023.00
Exif.CanonSi.BulbDuration Short 1 4035
Exif.CanonSi.CameraType Short 1 (54067)
696 rmills@rmillsmm-local:~/gnu/github/exiv2/main/build $ It appears that the metadata decoder is accessing the wrong bytes in this file. |
|
I'll wait for feedback from @mallman about my proposed fix before discovering how/why string I've investigated the file structure. exiv2, ExifTool and tvisitor all concur about the location of CanonSi (Shot Info) in the file. Therefore I will not undertake further study of the file. $ exiftool ../test/data/issue_1827_poc.crw | grep -e "Camera Type" -e "Focus Distance Upper"
Focus Distance Upper : 230.21 m
Camera Type : Unknown (-11469)
$ exiv2 -g CameraType -g SubjectDistance ../test/data/issue_1827_poc.crw
Exif.CanonSi.SubjectDistance Short 1 230.21 m
Exif.CanonSi.CameraType Short 1 (54067) == 0xffff-11469
$ tvisitor -pRU ../test/data/issue_1827_poc.crw | grep ShotInfo
0x102a | HW | 42 | Exif.CRW.CanonShotInfo | 54 | 12 | D$.9f..]1..m._OSU..o.r.u.K.[X$1..... +++
$ exiftool -v5 ../test/data/issue_1827_poc.crw | grep -i -A 2 ShotInfo
| | | 3) CanonShotInfo (SubDirectory) -->
| | | - Tag 0x102a (54 bytes, int16u[27]):
| | | 13ec: 44 24 28 39 66 e0 0a 5d 31 e1 92 6d 93 00 4f 53 [D$.9f..]1..m..OS]
$ |
|
It's amazing the stuff you find at the bottom of the swamp: std::ostream& CanonMakerNote::printSi0x0002(std::ostream& os,
const Value& value,
const ExifData*)
{
std::ios::fmtflags f( os.flags() );
if ( value.typeId() == unsignedShort
&& value.count() > 0) {
// Ported from Exiftool by Will Stokes
os << std::exp(canonEv(value.toInt64()) * std::log(2.0F)) * 100.0F / 32.0F;
}
os.flags(f);
return os;
}I've never heard of Wilf Stokes and even less idea what this code does. This code can write "inf". Proof? .../main/build $ git diff ../src/canonmn_int.cpp
diff --git a/src/canonmn_int.cpp b/src/canonmn_int.cpp
index 879a449d..b626e472 100644
--- a/src/canonmn_int.cpp
+++ b/src/canonmn_int.cpp
@@ -2877,7 +2877,7 @@ namespace Exiv2 {
if ( value.typeId() == unsignedShort
&& value.count() > 0) {
// Ported from Exiftool by Will Stokes
- os << std::exp(canonEv(value.toInt64()) * std::log(2.0F)) * 100.0F / 32.0F;
+ os << "wilf was here" ; // std::exp(canonEv(value.toInt64()) * std::log(2.0F)) * 100.0F / 32.0F;
}
os.flags(f);
return os;
.../main/build $ env DYLD_LIBRARY_PATH=$PWD/lib bin/exiv2 -pa -g iso/i ../test/data/issue_1827_poc.crw
Exif.CanonSi.AutoISO Short 1 2.00187e+06
Exif.CanonSi.ISOSpeed Short 1 wilf was here
.../main/build $ The command-line in the test is It does an amazing dance of writing the metadata into a stream and then parsing it again. Why? No idea! However, we now have answers to two questions:
index 0c851304..e41b7e6e 100644
--- a/src/easyaccess.cpp
+++ b/src/easyaccess.cpp
@@ -113,6 +113,7 @@ namespace Exiv2 {
std::ostringstream os;
md->write(os, &ed);
bool ok = false;
+ if ( os.str().find("inf") != std::string::npos ) break;
iso_val = parseInt64(os.str(), ok);
if (ok && iso_val > 0) break;
while (strcmp(keys[idx++], md->key().c_str()) != 0 && idx < cnt) {}This fix is "less aggressive" in that it's in the exiv2 application and not in the heart of the library. However, it's isolated to the single case of reporting weird ISOSpeed and nothing else. The immediate issue is to get exiv2/ASAN/macOS to pass the test suite. Two patches have been proposed:
@piponazo, @kevinbackhouse . Could you discuss this? I haven't really understood what the I prefer the low-level change because it solves the ISOSpeed issue and probably reinforces other parts of the exiv2 application. Damn Difficult Followup Question Why do we not see this on other platforms? Maybe apple/clang library issue? In the code that I replaced with "wilf was here", we could investigate how the following performs on other platforms: std::cout << std::exp(canonEv(value.toInt64()) * std::log(2.0F)) * 100.0F / 32.0F;I'm not offering to undertake such an investigation. To really do justice to this, we should investigate everything contributed by Wilf and the EasyAccess API. That's not a criticism of Wilf. I believe that code's been there for about 15 years and ExifTool may work in a different way today. |
|
The stringTo() template accepts "inf" as valid (and +Inf, and -Inf) and possibly other values. The stringTo() template is good and should not be modified. The issue is caused by the application code which writes and then parses. That's not working correctly when the string is "inf". md->write(os, &ed);
bool ok = false;
iso_val = parseInt64(os.str(), ok);This needs more time which I don't want to give. The second patch enables the test suite to run on macOS. 1165 rmills@rmillsmm-local:~/temp $ cat stringTo.cpp
#include <iostream>
#include <string>
#include <sstream>
int stringToInt(const std::string& s, bool& ok)
{
std::istringstream is(s);
int tmp ;
ok = (is >> tmp) ? true : false;
std::string rest;
is >> std::skipws >> rest;
if (!rest.empty()) ok = false;
return tmp;
}
float stringToFloat(const std::string& s, bool& ok)
{
std::istringstream is(s);
float tmp ;
ok = (is >> tmp) ? true : false;
std::string rest;
is >> std::skipws >> rest;
if (!rest.empty()) ok = false;
return tmp;
}
int main(int argc, const char* argv[])
{
for ( int a = 1 ; a < argc ; a++ ) {
std::string s(argv[a]);
bool oki,okf ;
int i = stringToInt(s,oki);
float f = stringToFloat(s,okf);
std::cout << a << " : " << s
<< " = " << i
<< " ok = " << oki
<< " = " << f
<< " ok = " << okf
<< std::endl;
}
return 0 ;
}
1166 rmills@rmillsmm-local:~/temp $ ./stringTo 10 20 inf foo X +inf
1 : 10 = 10 ok = 1 = 10 ok = 1
2 : 20 = 20 ok = 1 = 20 ok = 1
3 : inf = 0 ok = 0 = inf ok = 1
4 : foo = 0 ok = 0 = 0 ok = 0
5 : X = 0 ok = 0 = 0 ok = 0
6 : +inf = 0 ok = 0 = inf ok = 1
1167 rmills@rmillsmm-local:~/temp $ |
I agree, the EasyAccess API needs to be reviewed at some point. While working on the manpage, I found that some of the tags listed in isoSpeed() don't work correctly. In the When the EasyAccess API is reviewed, we may find other similar problems. However, as many new tags have been added to Exiv2, we may also find that some of them can be added to the EasyAccess lists. At the moment, I have too many other projects to commit to a full review of the EasyAccess API. |
|
@postscript-dev For sure, Peter, nobody has the time to get sucked into the EasyAccess API. @kmilos and I did some work on EasyAccess in 2020 and I documented it in the Wiki: https://github.com/Exiv2/exiv2/wiki/EasyAccess-API. I don't recall how we became interested in that in 2020. It's been there for a long time with almost no issues. In order to get the macOS/ASAN/x86_64 build to pass the test suite, I think we should adopt the change discussed here: #2123 (comment). We should not touch the good code in the stringTo() template. |
|
Thanks @clanmills for your investigation. I might take this challenge in the next days, it looks like an interesting one. I just hope I can reproduce some of the issues in other platforms. Otherwise, I might ask you access again to your mac PC 😉 |
|
For sure you are welcome to use the MacMini. You can access it using the IP address (email me to get that). I think there are at least three different issues:
The first is easy. index 0c851304..e41b7e6e 100644
--- a/src/easyaccess.cpp
+++ b/src/easyaccess.cpp
@@ -113,6 +113,7 @@ namespace Exiv2 {
std::ostringstream os;
md->write(os, &ed);
bool ok = false;
+ if ( os.str().find("inf") != std::string::npos ) break;
iso_val = parseInt64(os.str(), ok);
if (ok && iso_val > 0) break;
while (strcmp(keys[idx++], md->key().c_str()) != 0 && idx < cnt) {}@mallman. This fix only impacts EasyAccess/ISO. Can you update the PR with this change, please? The "inf" matter is indeed interesting and can probably be understood in a couple of days. I suspect the code is good and we only need changes to the ISO pretty printer and some Wiki documentation. If changes are needed, they should be in a different PR. Let's get the macOS/ASAN/x86_64 CI into production. ISO storage looks like a black hole that could consume a huge amount of time. The code has been there for a long time and nobody has complained. Until solid bugs are reported by users, we should leave this alone. |
1237 rmills@rmillsmm-local:~/temp $ cat stringTo.cpp
#include <iostream>
#include <string>
#include <sstream>
#include <iomanip>
#include <cmath>
#include <cerrno>
#include <cstring>
#include <cfenv>
int stringToInt(const std::string& s, bool& ok)
{
std::istringstream is(s);
int tmp ;
ok = (is >> tmp) ? true : false;
std::string rest;
is >> std::skipws >> rest;
if (!rest.empty()) ok = false;
return tmp;
}
float stringToFloat(const std::string& s, bool& ok)
{
std::istringstream is(s);
float tmp ;
ok = (is >> tmp) ? true : false;
std::string rest;
is >> std::skipws >> rest;
if (!rest.empty()) ok = false;
return tmp;
}
int64_t fib(uint64_t n)
{
return n <= 1 ? n : fib(n-1) + fib(n-2);
}
float canonEv(int64_t val)
{
// temporarily remove sign
int sign = 1;
if (val < 0) {
sign = -1;
val = -val;
}
// remove fraction
const auto remainder = val & 0x1f;
val -= remainder;
float frac = static_cast<float>(remainder);
// convert 1/3 (0x0c) and 2/3 (0x14) codes
if (frac == 0x0c) {
frac = 32.0F / 3;
}
else if (frac == 0x14) {
frac = 64.0F / 3;
}
else if ((val == 160) && (frac == 0x08)) { // for Sigma f/6.3 lenses that report f/6.2 to camera
frac = 30.0F / 3;
}
return sign * (val + frac) / 32.0F;
}
float wilf(uint64_t value)
{
return std::exp(canonEv(value) * std::log(2.0F)) * 100.0F / 32.0F;
}
int main(int argc, const char* argv[])
{
if ( argc == 1 ) {
for ( uint64_t a = 0 ; a < 50 ; a++ ) {
std::cout << a*100 << " -> " << wilf(a*100) << std::endl;
}
} else for ( int a = 1 ; a < argc ; a++ ) {
std::string s(argv[a]);
bool oki,okf ;
int i = stringToInt(s,oki);
float f = stringToFloat(s,okf);
std::cout << a << " : " << s
<< " = " << i
<< " ok = " << oki
<< " = " << f
<< " ok = " << okf
<< std::endl;
}
return 0 ;
}
1238 rmills@rmillsmm-local:~/temp $ make -B stringTo
c++ stringTo.cpp -o stringTo
stringTo.cpp:46:11: warning: 'auto' type specifier is a C++11 extension [-Wc++11-extensions]
const auto remainder = val & 0x1f;
^
1 warning generated.
1239 rmills@rmillsmm-local:~/temp $ ./stringTo
0 -> 3.125
100 -> 27.2627
200 -> 237.841
300 -> 2015.87
400 -> 18101.9
500 -> 162550
600 -> 1.37772e+06
700 -> 1.20194e+07
800 -> 1.04858e+08
900 -> 9.14784e+08
1000 -> 7.98063e+09
1100 -> 6.76414e+10
1200 -> 6.074e+11
1300 -> 5.45427e+12
1400 -> 4.62288e+13
1500 -> 4.03303e+14
1600 -> 3.51844e+15
1700 -> 3.06951e+16
1800 -> 2.67786e+17
1900 -> 2.26967e+18
2000 -> 2.03809e+19
2100 -> 1.83014e+20
2200 -> 1.55118e+21
2300 -> 1.35326e+22
2400 -> 1.18059e+23
2500 -> 1.02995e+24
2600 -> 8.98538e+24
2700 -> 7.61577e+25
2800 -> 6.83872e+26
2900 -> 6.14096e+27
3000 -> 5.20491e+28
3100 -> 4.5408e+29
3200 -> 3.96142e+30
3300 -> 3.45596e+31
3400 -> 3.015e+32
3500 -> 2.55543e+33
3600 -> 2.29469e+34
3700 -> 2.06055e+35
3800 -> 1.74647e+36
3900 -> inf
4000 -> inf
4100 -> inf
4200 -> inf
4300 -> inf
4400 -> inf
4500 -> inf
4600 -> inf
4700 -> inf
4800 -> inf
4900 -> inf
1240 rmills@rmillsmm-local:~/temp $ |
(Diff authored by clanmills)
|
I think the codecov failure was spurious. I re-ran it and it succeeded. |
|
@clanmills I've pushed a commit with the change you requested. |
|
Better. It's succeeded on macOS. Can you have a look at the two failures, please? They are probably either benign or incomprehensible and we can merge. |
|
I re-ran the codecov check and it succeeded. The other check fails because of some kind of configuration issue unrelated to this PR. I think it's safe to merge this PR. |
|
I concur. Can you invite Luis @piponazo to review and approve. I believe you have permission to merge. I've proposed a Team Exiv2 convention: The author of a PR is the person to merge. A few weeks ago, I merged one of Luis' PRs and he wasn't finished. Apple have a house convention that the person who opens an issue is the person to close it. It's a good way of saying "I'm satisfied.". |
Sounds like a good idea to me. |
This is a PR build workflow for macOS+sanitizers. As can be seen at #2117 (comment), two test failures are reported in this configuration (for x86_64).
I'm not sure how to test GitHub workflow changes. Let's see if creating this PR activates the workflow in this PR. If it doesn't, I'll look into how to test it independently.