https://bugs.kde.org/show_bug.cgi?id=524678
Bug ID: 524678
Summary: EXR: `isoSpeed` attribute from the file is passed to
`qRound()` unchecked, aborting on a Q_ASSERT
Classification: Frameworks and Libraries
Product: frameworks-kimageformats
Version First unspecified
Reported In:
Platform: Compiled Sources
OS: All
Status: REPORTED
Severity: crash
Priority: NOR
Component: general
Assignee: [email protected]
Reporter: [email protected]
CC: [email protected], [email protected]
Target Milestone: ---
Created attachment 195427
--> https://bugs.kde.org/attachment.cgi?id=195427&action=edit
Crashing Inputs, potential Fix
**Version:** master `458257b7a32417c995d3a95a65b38586eee7a91a`
**Plugin:** `src/imageformats/exr.cpp`
Reading an EXR whose `isoSpeed` header attribute holds a value outside `int`
range
aborts the process on a Qt assertion. The file is otherwise completely valid
and decodes
normally — only the metadata value is hostile.
## Problem
`readMetadata()` rounds the attribute straight to an integer —
`exr.cpp:370-372`:
```c++
if (auto isoSpeed = header.findTypedAttribute<Imf::FloatAttribute>("isoSpeed"))
{
image.setText(QStringLiteral(META_KEY_ISOSPEEDRATINGS),
QLocale::c().toString(qRound(isoSpeed->value()))); // line
371
}
```
`isoSpeed->value()` is a `float` read verbatim from the file. Qt's `qRound()`
goes
through `QtPrivate::qCheckedFPConversionToInteger`, which asserts the value is
representable (`qnumeric.h:508`, `:517`, `:523`):
```c++
Q_ASSERT(!std::isnan(value));
Q_ASSERT(value - FP(minimal) > FP(-1));
Q_ASSERT(value < maximalPlusOne); // <-- fires
```
NaN, ±infinity, and any magnitude beyond `INT_MAX` all reach it, and an EXR
float
attribute may legally hold any of them.
Note this is *not* caught by the `catch (const std::exception &)` around
`EXRHandler::read()` — `Q_ASSERT` calls `qFatal()` → `abort()`, which is not an
exception. And because `readMetadata()` runs after the pixel loop
(`exr.cpp:493`), the
file must be fully valid and decodable to get there, which is why structurally
malformed inputs never surface this.
## Stack
>From an ASan build (oss-fuzz `kimgio_exr_fuzzer`):
```
#2 qAbort() qassert.cpp:47
#6 qt_assert(char const*, char const*, int) qassert.cpp:118
#7 qCheckedFPConversionToInteger<int, float, true, true> qnumeric.h:523
#8 qRound qnumeric.h:577
#9 readMetadata(Imf_3_4::Header const&, QImage&) exr.cpp:371
#10 EXRHandler::read(QImage*) exr.cpp:493
```
## Reproducing
Two files are attached:
- **`crash.exr`** (1447 bytes) — the original input from the fuzzer. Known to
reproduce.
- **`minimal.exr`** (358 bytes) — a 1×1 RGB half, uncompressed image whose
*only*
unusual property is `isoSpeed`. Decodes fully, then aborts. Generated by the
attached
`make_minimal.py`; `--iso nan` and `--iso inf` produce the other trigger
values.
Both set `isoSpeed` to the bytes `8f 4e cd 7a`, i.e. **5.33007e+35**.
```c++
QImage img;
img.load("minimal.exr"); // aborts in a build with asserts enabled
```
Reproduced directly against Qt 6.11.1 with the value taken from the file, no
fuzzer and
no sanitizer involved:
```
$ ./repro_qround
isoSpeed = 5.33007e+35 (INT_MAX = 2147483647)
ASSERT: "value < maximalPlusOne" in file
.../QtCore.framework/Headers/qnumeric.h, line 523
```
### Behaviour without asserts
`Q_ASSERT` is compiled out under `QT_NO_DEBUG`, so distribution builds do not
abort:
```
$ ./repro_qround_rel # -O2 -DQT_NO_DEBUG -DNDEBUG
isoSpeed = 5.33007e+35
qRound = 0
```
Worth noting that converting an unrepresentable float to `int` is undefined
behaviour in
C++, not merely lossy — `0` is what this compiler produced, not a guarantee. So
the
impact is an abort in assert-enabled builds (CI, debug, oss-fuzz) and a
meaningless ISO
value in the image metadata otherwise.
## Fix
kimageformats already has the helper for this, and already uses it in the same
function —
though its upper bound needs a one-character correction, see below.
`util_p.h:169`:
```c++
TI qRoundOrZero_T(SF d, bool *ok = nullptr)
{
// checks for undefined behavior
if (qIsNaN(d) || qIsInf(d) || d < SF() || d >
SF(std::numeric_limits<TI>::max())) {
if (ok) { *ok = false; }
return 0;
}
if (ok) { *ok = true; }
return qRound(d);
}
```
`xDensity` is run through it via `dpi2ppm()` forty lines above the crash
(`exr.cpp:327-339`). The `isoSpeed` branch just does not follow that pattern.
```diff
--- a/src/imageformats/exr.cpp
+++ b/src/imageformats/exr.cpp
@@ -368,7 +368,15 @@ static void readMetadata(const Imf::Header &header, QImage
&image)
// shot metadata
if (auto isoSpeed =
header.findTypedAttribute<Imf::FloatAttribute>("isoSpeed")) {
- image.setText(QStringLiteral(META_KEY_ISOSPEEDRATINGS),
QLocale::c().toString(qRound(isoSpeed->value())));
+ // The value comes straight from the file: it may be NaN, infinite or
outside
+ // the range of int, all of which make qRound() assert (and are
undefined
+ // behavior once asserts are compiled out). qRoundOrZero() performs
the same
+ // checks already used for xDensity above.
+ bool ok = false;
+ auto iso = qRoundOrZero(isoSpeed->value(), &ok);
+ if (ok) {
+ image.setText(QStringLiteral(META_KEY_ISOSPEEDRATINGS),
QLocale::c().toString(iso));
+ }
}
```
Using the `ok` flag rather than the zero return means an unusable value skips
the tag
rather than recording `ISO 0`. No new includes required.
### The helper's upper bound is off by one
`qRoundOrZero_T` rejects `d > SF(std::numeric_limits<TI>::max())`. That bound
is not
representable in the source float type and **rounds up**: `float(INT32_MAX)` is
exactly
`2^31`, one more than `INT_MAX`. With `>`, that rounded-up value passes the
guard,
reaches `qRound()`, and trips `Q_ASSERT(value < maximalPlusOne)` — where
`maximalPlusOne`
is that same `2^31`.
So `isoSpeed = 2147483648.0f` (bytes `4F 00 00 00`) still aborts with the
helper applied.
Verified against Qt 6.11.1 and OpenEXR 3.4.14:
| isoSpeed | upstream | helper as-is (`>`) | helper with `>=` |
|---|---|---|---|
| 800 | tag `800` | tag `800` | tag `800` |
| 5.33e35 | abort 134 | tag skipped | tag skipped |
| NaN | abort 134 | tag skipped | tag skipped |
| **2^31** | **abort 134** | **abort 134** | **tag skipped** |
This also affects the branch that was already guarded: `xDensity` reaches the
same helper
through `dpi2ppm()` (`util_p.h:202`), and `xDensity = 54546084.0f` (bits
`0x4C5013A9`)
makes `dpi / 25.4f * 1000.0f` come out as exactly `2^31`, aborting at
`exr.cpp:332` —
before the `isoSpeed` branch is even reached. `qint64` has the same shape,
since
`double(INT64_MAX)` is exactly `2^63`.
Hence the second hunk:
```diff
// checks for undefined behavior
- if (qIsNaN(d) || qIsInf(d) || d < SF() || d >
SF(std::numeric_limits<TI>::max())) {
+ if (qIsNaN(d) || qIsInf(d) || d < SF() || d >=
SF(std::numeric_limits<TI>::max())) {
```
With that, the guard was checked against every class of float an EXR attribute
can hold:
5.33e35, NaN, ±inf, ±FLT_MAX, `float(INT_MAX)` and `2^31` are all rejected; 800
→ `800`,
12.5 → `13`, 0 → `0`.
## What was and was not verified
**Verified end to end**, with real OpenEXR 3.4.14 and Qt 6.11.1, no fuzzer and
no
sanitizer. `repro_plugin.cpp` mirrors `EXRHandler::read()` —
`Imf::RgbaInputFile`,
the scanline loop, then the `readMetadata()` conversion:
```
$ ./repro_plugin minimal.exr
decoded header : 1x1
decoded pixels : ok (1 scanline(s))
isoSpeed : 5.33007e+35
ASSERT: "value < maximalPlusOne" in file .../qnumeric.h, line 523
exit=134
$ ./repro_plugin crash.exr
decoded header : 4x4
decoded pixels : ok (4 scanline(s))
isoSpeed : 5.33007e+35
ASSERT: "value < maximalPlusOne" in file .../qnumeric.h, line 523
exit=134
$ ./repro_plugin minimal_nan.exr
decoded header : 1x1
decoded pixels : ok (1 scanline(s))
isoSpeed : nan
ASSERT: "!std::isnan(value)" in file .../qnumeric.h, line 508
exit=134
```
Both files decode completely — header parsed, all scanlines read — before the
abort,
confirming this is reached only by a *valid* EXR and that `minimal.exr` is a
genuine
1x1 image rather than merely well-shaped bytes. The NaN variant trips the other
assert
(line 508), so both of Qt's preconditions are reachable from a file.
**The fix verified on the same files** (`repro_plugin_fixed.cpp`, using
`qRoundOrZero_T` from `util_p.h:169` with the corrected `>=` bound):
| file | isoSpeed | before | after |
|---|---|---|---|
| `minimal.exr` | 5.33e35 | abort, exit 134 | loaded, tag skipped |
| `crash.exr` | 5.33e35 | abort, exit 134 | loaded, tag skipped |
| `minimal_nan.exr` | NaN | abort, exit 134 | loaded, tag skipped |
| `minimal_ok.exr` | 800 | loaded, tag `800` | loaded, tag `800` |
Normal values are unaffected; only unusable ones lose the tag.
**Still not verified:** the `QImageReader` plumbing around the plugin — the
plugin
itself was not built, since that needs the KDE build stack. `repro_plugin.cpp`
reproduces `EXRHandler::read()`'s logic against the same libraries, and the
fuzzer stack
covers the `QImageReader` half.
## Possibly related
`src/imageformats/microexif.cpp:359` has the same shape on the write path
(`ds << T(qRound(v * den))`, where `rationalPrecision()` bottoms out at
`10^0`), so a
large enough value reaches `qRound()` out of range there too. Not tested for
reachability from a crafted input — mentioned only in case it is useful.
---
Best regards,
The Fandango Cispa Team
--
You are receiving this mail because:
You are watching all bug changes.
