X Tutup
Skip to content

GRAPHICS: Handle seek-only case in TTFLibrary readCallback#6324

Merged
bluegr merged 2 commits intoscummvm:masterfrom
tunnelsociety:stream-read-nullptr
Dec 19, 2024
Merged

GRAPHICS: Handle seek-only case in TTFLibrary readCallback#6324
bluegr merged 2 commits intoscummvm:masterfrom
tunnelsociety:stream-read-nullptr

Conversation

@tunnelsociety
Copy link
Contributor

@tunnelsociety tunnelsociety commented Dec 19, 2024

The FreeType API Reference explains that a FT_Stream_IoFunc such as our TTFLibrary::readCallback should simply perform a seek if count is 0 and return a status.

In that case, buffer can be nullptr; previously, that was passed to a stream's read(), which in turn gave it to memcpy(). Some argue that memcpy(null, ..., 0) is acceptable, but it's undeniably undefined behaviour.

This PR includes a commit which checks nullptr in MemoryReadStream::read. I guess that ought never to be the case, so it may be dropped, but it has happened:

common/stream.cpp:119:8: runtime error: null pointer passed as argument 1, which is declared to never be null

#0  Common::MemoryReadStream::read (this=0x60700002dac0, dataPtr=0x0, dataSize=0) at common/stream.cpp:113
#1  0x0000555559c3d3c2 in Common::File::read (this=<optimized out>, ptr=0x0, len=0) at common/file.cpp:142
#2  0x00005555595abfa9 in Graphics::TTFLibrary::readCallback (stream=<optimized out>, offset=<optimized out>, buffer=0x0, count=0)
    at graphics/fonts/ttf.cpp:153
#3  0x00007ffff7570ccb in ?? () from /lib/x86_64-linux-gnu/libfreetype.so.6
[...]
#8  0x00005555595af6ab in Graphics::TTFLibrary::loadFont (this=this@entry=0x603000018070, ttfFile=<optimized out>, 
    stream=stream@entry=0x61800000bc90, face_index=face_index@entry=0, face=@0x61800000bce0: 0x0) at graphics/fonts/ttf.cpp:141

...sheesh, I'm switching to plain -O0...

As documented in FreeType API Reference. This avoids passing
a null buffer to ttfFile->read().
Even with dataSize=0, this is undefined behaviour.
What caller would pass in nullptr? UBSan caught this happening in a
FreeType callback, for example.
@bluegr
Copy link
Member

bluegr commented Dec 19, 2024

Thanks!

@bluegr bluegr merged commit 30661a9 into scummvm:master Dec 19, 2024
@tunnelsociety
Copy link
Contributor Author

Shoot, I've just noticed the code formatting is not good, sorry! I guess it can simply be fixed in a janitorial pass in the future.

@tunnelsociety tunnelsociety deleted the stream-read-nullptr branch December 19, 2024 20:58
@bluegr
Copy link
Member

bluegr commented Dec 20, 2024

Shoot, I've just noticed the code formatting is not good, sorry! I guess it can simply be fixed in a janitorial pass in the future.

Thanks, fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

X Tutup