bug: get_image_info returns true after copying an uninitialised buffer into the caller's output when every UDIM tile is unreadable
メンテナーはふだん 2 日以内に返信
評価
- 難易度
- 2/5
- 見積もり時間
- 1〜3時間
- 初心者へのやさしさ
- 76/100
- issue の種類
- バグ
- 明瞭さ
- 明確に書かれている
- 活発さ
- 活発
- 技術スタック
- cpp
調査の方向性
src/libtexture/imagecache.cpp の UDIM 分岐を読み、特に first フラグと最後の memcpy および戻り値を確認してください。すべてのタイルを開けない UDIM セットに対して ImageCache::get_image_info を直接呼び出し、動作を確認してください。issue には再現手順と期待される結果が含まれています。どのタイルからも値が得られなかった場合、呼び出しが false を返し、呼び出し元の出力を上書きしないことを完了条件とします。
索引モデルが issue の本文から書いたものです。
説明
Describe the bug
ImageCacheImpl::get_image_info aggregates an attribute across the tiles of a
UDIM set. If every tile it finds is unreadable, it copies an internal buffer
that nothing ever wrote into the caller's output and returns true. The
caller's variable is overwritten, with whatever the stack held.
src/libtexture/imagecache.cpp, at main:
if (file->is_udim()) { // :3111
size_t size = datatype.size(); // :3115
char* common = OIIO_ALLOCA(char, size); // :3116
char* single = OIIO_ALLOCA(char, size); // :3117
bool first = true; // :3118
...
if (concretefile && !concretefile->broken()) { // :3128
bool r = get_image_info(concretefile, ..., // :3132
first ? common : single);
if (!r) {
// If we couldn't retrieve the attribute from at
// least one existing file, it's a failure.
(void)geterror(); // :3138
return false; // :3139
}
if (first) {
first = false; // :3142
...
}
// If we got this far, all the tiles we found had the attribute, and
// they all had the same value. Copy to the user and return success.
memcpy(data, common, size); // :3153
return true; // :3154
}
Note that the inner failure path at :3135-3140 is correct: a tile that opens
but cannot answer the query fails the whole call. The defect is the other path.
OIIO_ALLOCA does not initialise its buffer, and the guard at :3128 has no
else, so a tile that is already broken is skipped in silence. When every tile
is skipped that way the inner get_image_info is never called, first stays
true, common is never written, and :3153 copies it to the caller under a
true return. The comment at :3151 states the postcondition the code does
not establish: there were no tiles that had the attribute. The one-line version
of the defect is that the closing memcpy and return true are not predicated
on !first.
Every tile in the set has to fail, and it has to fail at open rather than
later. UDIM sets are inventoried by directory listing and name matching alone,
so any file sitting at a UDIM index joins the set regardless of its contents,
and a zero-byte, wrong-format or header-damaged one joins it and then fails to
open. A file whose header survives does open, so a write truncated after the
header is complete does not show this.
Evidence
Every tile below comes from the public corpora rather than from a file written
for the occasion. The damaged tiles are OpenEXR's own fuzzer corpus,
openexr-images/Damaged/, whose files carry the real EXR magic and fail during
header parse, not at format detection. The readable tile is
openexr-images/DisplayWindow/t01.exr, 400 x 300, 3 channel half. The corpus
files ship without a .exr extension, so each was copied to a tile name first.
Queried through ImageCache::get_image_info, with 0x5A5A5A5A in the caller's
int and a string literal in the caller's const char* beforehand:
| tile set | subimages | fileformat |
|---|---|---|
one damaged tile, asan_heap-oob_504235_131_… |
true, wrote -701499600 |
true, pointer …D62FF790 |
| two damaged tiles, different files | true, wrote 769522064 |
true, pointer …2DDDF9F0 |
| one tile of 30 ASCII bytes | true, wrote -659911935 |
true, pointer …C96FFD80 |
| one zero-byte tile | true, wrote 357628192 |
true, pointer …1550F980 |
| one tile truncated to 200 bytes | true, wrote -1464863040 |
true, pointer …A8AFF720 |
| the same damaged tile, queried by its concrete name | false, sentinel intact |
false, sentinel intact |
| one readable tile | true, wrote 1 |
true, "openexr" |
| readable tile at 1001, damaged tile at 1002 | true, wrote 1 |
true, "openexr" |
The last three rows are the controls. Queried by its concrete name the damaged
file returns false and leaves the caller's variable at the sentinel, which is
the correct behaviour and the contrast that matters. A set with anything
readable in it answers correctly, whichever index the readable tile sits at.
Only when every tile fails to open does the aggregate report success.
The reason is in geterror() in the failing cases, but a caller has no reason
to look after a true return.
What reaches the caller is an address, not a zero
The value is neither zero nor stable. Six fresh processes on one fixture:
subimages returned=1 value=615511760 fileformat pointer=0000001C24AFF730
subimages returned=1 value=-1768883216 fileformat pointer=000000829690FC50
subimages returned=1 value=-45090496 fileformat pointer=0000009CFD4FF9A0
subimages returned=1 value=-751829952 fileformat pointer=00000042D32FFCA0
subimages returned=1 value=-674236144 fileformat pointer=0000005ED7CFF910
subimages returned=1 value=1096611904 fileformat pointer=000000B8415CF8A0
The int is consistently 0x60 below the pointer, and both lie inside the
caller's own frame. Recording the address of a local in main() and comparing
against it:
subimages value=-701499600 -> main()'s stack, anchor-192 bytes
fileformat pointer=…D62FF790 -> main()'s stack, anchor-96 bytes
So what the caller gets back is a live address out of its own stack, differing
run to run with ASLR. On 3.2.0.2dev and 3.1.12 the fileformat pointer is a
heap address instead.
fileformat is the sharpest case. It is a TypeString query, so the caller
passes the address of a const char* and is expected to dereference what comes
back. A caller that checks the pointer for null before dereferencing passes
that check and then reads an arbitrary location.
Any attribute name reaches it, including one that is not supported
channels returns the same uninitialised value. So does udim, which the
implementation does not handle at all: imagecache.h:613-614 documents the
name in lowercase and imagecache.cpp:72 interns "UDIM".
every tile unreadable: udim returned=1, wrote -701499600
readable set: udim returned=0, sentinel intact
On a working set the unsupported name correctly returns false. On an
all-unreadable set it returns success with the uninitialised buffer, so the
aggregate answers a question it does not recognise.
exists and broken are resolved before this branch and are unaffected,
though they sit oddly beside the rest: on a set whose every tile is unreadable,
exists returns 1 and broken returns 0.
OpenImageIO version and dependencies
OIIO 3.2.2.0 | Windows/x86_64
Build compiler: MSVS 1951 | C++17/199711
HW features enabled at build: sse2,sse3,ssse3,sse41,sse42
No CUDA support (disabled / unavailable at build time)
Dependencies: BZip2 1.0.8, DCMTK NONE, FFmpeg NONE, fmt 12.1.0, Freetype 2.13.3, GIF 5.2.2, Imath 3.2.2, JXL NONE, libdeflate 1.25, Libheif NONE, libjpeg-turbo 3.1.3, LibRaw NONE, libuhdr NONE, OpenColorIO 2.5.1, OpenCV NONE, OpenEXR 3.4.7, OpenJPEG 2.5.4, openjph 0.26.3, PNG 1.6.55, Ptex NONE, Ptex NONE, Robinmap 1.4.1, TBB NONE, TIFF 4.7.1, WebP 1.6.0, ZLIB 1.3.1, zstd 1.5.7
The figures in the evidence above come from a source build of main at
3e8576595, so that the line numbers cited and the behaviour measured are the
same tree:
OIIO 3.3.0.0dev | Windows/x86_64
Build compiler: MSVS 1951 | C++17/199711
Dependencies: OpenEXR 3.4.7, Imath 3.2.2, fmt 12.1.0, libdeflate 1.25, ZLIB 1.3.1
That build was configured with OIIO_BUILD_TESTS=ON, USE_PYTHON=OFF,
STOP_ON_WARNING=OFF and OIIO_HARDENING=1, Release. None of those bear on
this: there is no assertion involved, only an uninitialised buffer that is
copied out.
The same fixture behaves identically on 3.1.12, on 3.2.0.2dev and on main
at 3e8576595: returned=1 with a varying nonzero value in every case.
oiiotool crashes on it, and only on it. oiiotool --info 'tex.<UDIM>.exr'
prints nothing and terminates with 0xC0000409 when every tile is unreadable.
Every other UDIM case exits 1 with a clear message, which isolates the crash to
this defect rather than to UDIM handling in general:
tiles exit output
all unreadable 0xC0000409 none
one readable tile 1 Cannot retrieve ImageSpec of a UDIM-like virtual file
one tile truncated after its header 1 Cannot retrieve ImageSpec of a UDIM-like virtual file
one readable tile plus one unreadable 1 ... EXR_ERR_FILE_BAD_HEADER ... tex.1002.exr
The partial-set row is worth noting: there oiiotool names the offending tile.
A cacheless ImageBuf does not reach the aggregate, because it opens through
ImageInput, which cannot open a UDIM pattern. The clean library-level
demonstration is a direct ImageCache caller.
A cache-backed ImageBuf issues exactly the subimages, miplevels and
fileformat queries above, but its init_spec then returns false for a
separate reason, "Cannot retrieve ImageSpec of a UDIM-like virtual file",
which fires on a readable set too. What does surface on the ImageBuf is the
uninitialised value itself:
nsubimages() -> 1483733088
file_format_name() -> "`\xefoXc"
geterror() -> "Invalid subimage 0 requested (of -647316976 subimages)"
To Reproduce
Using a real damaged image from OpenEXR's own corpus. Any of the files in
openexr-images/Damaged/ that fails to open will do, and 11 of the first 14
do; this one fails with EXR_ERR_MISSING_REQ_ATTR 'channels' attribute not found. They are stored without a .exr extension, hence the copy.
mkdir udim && cd udim
cp .../openexr-images/Damaged/asan_heap-oob_504235_131_78b0e15388401193e1ee2ce20fb7f3b0_exr \
tex.1001.exr
#include <OpenImageIO/imagecache.h>
#include <cstdio>
int main()
{
auto ic = OIIO::ImageCache::create(false);
int n = -12345;
const char* fmt = "UNTOUCHED";
bool a = ic->get_image_info(OIIO::ustring("tex.<UDIM>.exr"), 0, 0,
OIIO::ustring("subimages"), OIIO::TypeInt, &n);
bool b = ic->get_image_info(OIIO::ustring("tex.<UDIM>.exr"), 0, 0,
OIIO::ustring("fileformat"), OIIO::TypeString,
&fmt);
std::printf("subimages -> %s, wrote %d\n", a ? "true" : "false", n);
std::printf("fileformat -> %s, pointer %p, string \"%s\"\n",
b ? "true" : "false", (const void*)fmt,
fmt ? fmt : "(null)");
std::printf("geterror -> \"%s\"\n", ic->geterror().c_str());
return 0;
}
Expected: both return false, n stays -12345, fmt still reads
UNTOUCHED, and the reason is in geterror(). That is what the same file
gives when queried by its concrete name.
Observed, three fresh runs on main at 3e8576595:
subimages -> true, wrote -659911935 fileformat -> true, pointer 00000095C96FFD80
subimages -> true, wrote -492061951 fileformat -> true, pointer 00000030252FF650
subimages -> true, wrote 427505921 fileformat -> true, pointer 000000BAE336FD20
Both calls report success, the int sentinel is replaced by a different value
each run, and the valid const char* is replaced by an address the caller is
expected to dereference. Linking needs OpenImageIO and OpenImageIO_Util.
The fix
Return false when no tile supplied a value, rather than copying a buffer that
was never written:
if (first)
return false; // no readable tile had the attribute
memcpy(data, common, size);
return true;
first is already the exact condition, so this is one line.
A fuller answer would also report the reason. The two paths handle errors in
opposite directions and neither suits the skipped-tile case. The inner failure
path at :3138 clears the cache's whole error string before returning false,
and the skipped-tile path at :3128 clears nothing, so the call returns true
with the tile's EXR error still queued. That is visible in the evidence above:
geterror() holds EXR_ERR_FILE_BAD_HEADER after a call that returned
success. Reporting the first skipped tile's error on the all-skipped path would
turn a silent wrong answer into a diagnosable one.
Two in-tree consumers use the result without checking it. Both are reached
on ordinary paths, which is why this is worth more than a wrong return value.
ImageBuf::init_spec interns the copied-out pointer before validating
anything, src/libOpenImageIO/imagebuf.cpp:1176:
m_fileformat = ustring(fmt);
The three queries above it at :1161-1167 all return true here, so the error
branch is skipped and fmt arrives holding the uninitialised bytes.
ustring(const char*) is null-safe but not garbage-safe,
src/include/OpenImageIO/ustring.h:140-143:
explicit ustring(const char* str)
{
m_chars = str ? make_unique(str) : nullptr;
}
so a non-null wild pointer goes to make_unique, which means strlen on an
arbitrary address. A zero would have been safe. An address is not.
testtex discards the return value outright and compares the pointer,
src/testtex/testtex.cpp:1962-1966:
const char* texturetype = "Plain Texture";
texsys->get_texture_info(filename, 0, ustring("texturetype"),
TypeDesc::STRING, &texturetype);
Timer timer;
if (!strcmp(texturetype, "Plain Texture")) {
No flag is needed to reach that, and on an all-unreadable set it is a strcmp
against uninitialised stack.
Test coverage
testsuite/texture-udim builds its tiles with oiiotool -otex and queries
them with testtex --gettextureinfo, so every tile is always readable. A
regression test would add one unreadable file at a UDIM index and assert that
the aggregate query returns false rather than true.
- 主要言語
- C++
- スター
- 2.4k
- フォーク
- 718
- 平均マージ
- 2日 18時間
- マージ済み PR(30日)
- 85
環境構築
- Dockerfile・Docker Compose ファイルなし
- プルリクエストのテンプレートあり
- コントリビューションガイドを読む
はじめの一歩
- issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
- 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
- リポジトリをフォークし、ブランチを切って変更します。
- issue 番号を参照したプルリクエストを送ります。
AcademySoftwareFoundation/OpenImageIO のほかの issue
-
bug: copy() writes destination channels the source does not have, taking the values from past the source's pixel対応中かも @lgritz が 1 日前に担当しました。 オープン
難易度 2/5 1〜3時間 初心者へのやさしさ 35/100
AcademySoftwareFoundation/OpenImageIO#5661 · コメント 1 件 · 担当者 1 名 ·
メンテナーはふだん 2 日以内に返信
-
bug: a malformed filename throws std::regex_error out of ImageCache, and catching it deadlocks the file cache対応中かも @lgritz が 2 日前に担当しました。 オープン
AcademySoftwareFoundation/OpenImageIO#5660 · 担当者 1 名 ·
メンテナーはふだん 2 日以内に返信
-
bug: deep paste wraps into the next scanline instead of clipping, and ignores chbegin対応中かも @lgritz が 1 日前に担当しました。 オープン
難易度 4/5 3〜5日 初心者へのやさしさ 45/100
AcademySoftwareFoundation/OpenImageIO#5649 · コメント 1 件 · 担当者 1 名 ·
メンテナーはふだん 2 日以内に返信
-
bug: make_kernel("binomial", ...) indexes its tap arrays past their allocation for even or fractional extents対応中かも @lgritz が 4 日前に担当しました。 オープン
難易度 3/5 半日 初心者へのやさしさ 62/100
AcademySoftwareFoundation/OpenImageIO#5648 ·
メンテナーはふだん 2 日以内に返信
-
難易度 5/5 1週間以上 初心者へのやさしさ 35/100
AcademySoftwareFoundation/OpenImageIO#5619 · コメント 2 件 ·
メンテナーはふだん 2 日以内に返信
AcademySoftwareFoundation/OpenImageIO の issue をすべて見る
似ている issue
-
ai_p2
難易度 2/5 1〜3時間 初心者へのやさしさ 82/100
ClickHouse/ClickHouse#130006 ·
メンテナーはふだん 1 日以内に返信
-
DepotMaintain: EN longer string moves the hint popup buttons out of reach // WpfGUI English Languageオープンclient: WpfGui module: depot question
難易度 2/5 1〜3時間 初心者へのやさしさ 64/100
MaaAssistantArknights/MaaAssistantArknights#19169 · コメント 2 件 ·
メンテナーはふだん 1 日以内に返信
-
area: certificates bug outdated-version
難易度 1/5 1時間未満 初心者へのやさしさ 65/100
RafaelNGP/xbox-cloud-gaming-ps5#381 · コメント 1 件 ·
メンテナーはふだん 1 日以内に返信
-
難易度 2/5 1〜3時間 初心者へのやさしさ 85/100
メンテナーはふだん 1 日以内に返信
-
難易度 2/5 1〜3時間 初心者へのやさしさ 77/100
apache/iceberg-cpp#1103 ·
メンテナーはふだん 2 日以内に返信