You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Reported in version: 2.0.12 Reported for operating system, platform: Linux, x86_64
Comments on the original bug report:
On 2014-10-10 07:23:50 +0000, Iris Morelle wrote:
Created attachment 1893
Patch against SDL_ttf @ f28f7b0f72ae
Under certain circumstances, the TTF_RenderUTF8* function family (also used by their TTF_RenderUNICODE* and TTF_RenderText* counterparts in SDL_ttf 2.0.12), may read and write to memory preceding an allocated pixmap block, potentially corrupting other structures and causing execution to crash later at a random point, especially during SDL invocations -- either by tripping a libc sanity check ("free(): invalid size" aborts, etc.), or causing a plain segmentation fault.
The affected (base) functions I could identify from runtime testing with valgrind's memcheck tool are:
TTF_RenderUTF8_Blended
TTF_RenderUTF8_Shaded
TTF_RenderUTF8_Solid
From a cursory glance at the code, I suspect TTF_RenderUTF8_Blended_Wrapped is affected as well since it uses the same pattern for copying the glyph from FreeType into the target SDL_Surface's pixmap.
The problematic pattern in question:
SDL_Surface *textbuf;
c_glyph *glyph;
int offset;
Uint32 *dst_check;
/* glyph->minx may be negative and less than -offset below! */
Uint32 *dst = (Uint32*) textbuf->pixels + offset + glyph->minx
/* (dst < dst_check) is verified later, but (textbuf->pixels >= dst) isn't */
The circumstances for triggering the fault are, unfortunately, very specific:
Using the DejaVu Sans font at size 16 to render...
A string consisting of an ASCII space followed by a Unicode combining character (U+0361 COMBINING DOUBLE INVERTED BREVE in my tests)
Potentially system-specific parameters:
X.org server on Linux configured for 96x96 dpi
Debian testing, with FreeType 2.5.2
(The resulting bug in our game has been confirmed by at least two people running other Linux distributions, so I'm tempted to assume it isn't caused by a single FreeType version at least.)
I initially observed and ascertained this bug with SDL_ttf 2.0.11 and SDL 1.2.15, since it's (unfortunately) what our affected game still uses. However, I later moved to the mercurial repo tips (SDL_ttf 2.0.12+ at commit f28f7b0f72ae, SDL 2.0.3+ at commit 9ecf775ead1b) for reviewing my initial analysis, writing a test case, and the patch.
Although I only ran tests against the current SDL_ttf codebase, a quick look at the released 2.0.12 source suggests that it is also affected.
I am attaching to this report:
A patch against SDL_ttf.c from the current hg repo tip that solves the issue for me, dealing with the aforementioned four TTF_RenderUTF8* functions.
A small stand-alone test case for this bug (well, mostly -- it requires a copy of the DejaVuSans.ttf font from http://dejavu-fonts.org/) including instructions for building and running it
A log of what valgrind has to say for the test case before applying the patch to SDL_ttf.
The patch amends the pattern I described before so that the dst pointer is guaranteed to be within the textbuf->pixels block even if a negative offset comes into play.
(It's worth noting that I am NOT at all familiarized with SDL_ttf's API or internals, and this is all the product of my research over the course of the last two days while trying to hunt a resulting crash bug in our game, reported by a user with a considerably more elaborate test case.)
I decided to submit the bug against the most recent version of the library since it's bound to cause problems for somebody else some day. Seeing as how the maintainers kept the first two version numbers for the SDL 2 port, I imagine it's not their intention to update the 1.2 port again. Still, may I ask whether a fixed release for the 1.2.x version would be feasible? I do have an alternate patch that deals with the issue for SDL_ttf 2.0.11 (the TTF_RenderUNICODE* functions served as the implementation detail in 2.0.11 instead).
On 2014-10-10 07:24:41 +0000, Iris Morelle wrote:
Created attachment 1894
Standalone test case
On 2014-10-10 07:25:17 +0000, Iris Morelle wrote:
Created attachment 1895
Valgrind memcheck output without the patch applied
On 2014-10-14 19:01:44 +0000, Sylvain wrote:
Hi,
I gave a look at this issue. I can reproduce it with the latest version of the trunk. It needs exactly the specified font. Another font wouldn't have necessary the same problem.
Which points out an issue when the first glyph has a negative "minx" value for drawing.
Here, this is the "second" glyph that has a negative "minx" value. And this value is bigger than the offset of "first" renderer glyph. So, when rendering the second glyph there an invalid read/write.
I would say the patch is not perfect because: it indirectly rounds the minx, instead of foreseeing the correct "startx" offset. So rendering is not perfect.
And also, to make it perfect we should make sure also that the third glyph hasn't a minx big enough neither. And so on for all glyphs. Which would make the detection be totally crazy.
Another solution would be to have a buffer with a left side padding. But that adds a (slow) processing step afterwards.
So, the patch of simply checking that the correctness of the dst pointer is good compromise.
On 2014-10-20 01:03:51 +0000, Iris Morelle wrote:
Indeed the patch is not perfect. Since I've never used FreeType before and I'm not familiarized with SDL_ttf's codebase to that level, I decided to focus on solving the immediate memory corruption bug rather than attempt to ensure render correctness -- especially since the issue has rather grave implications for our software.
On 2014-10-24 13:14:38 +0000, Sylvain wrote:
I think the patch is fine.
It's not perfect, but it's a good compromise between a crazy detection of invalid negative sum of minx, to solve an issue that will very likely never occurs.
On 2014-10-25 02:34:31 +0000, Chris Beck wrote:
"It's not perfect, but it's a good compromise between a crazy detection of invalid negative sum of minx, to solve an issue that will very likely never occurs."
For what it's worth I disagree. I don't see how this is "crazy detection", surely the cost of scanning the input and calculating this is negligible compared to the cost of rendering the text. If you just think it's hard to write this, it seems that it would be very easy to write something that checks the pointer bounds with every write. This library is already quite fast, I think even that overhead would be preferable to the status quo, which is obviously a security compromise for any application that uses this function with unicode text coming from the user.
The kinds of users who want to use bugs like this to crash other users' clients in a multiplayer game surely won't be dissuaded by having to paste a slightly longer unicode sequence.
On 2014-10-31 11:02:34 +0000, Sylvain wrote:
(In reply to Chris Beck from comment # 6)
... If you just think it's hard to
write this, it seems that it would be very easy to write something that
checks the pointer bounds with every write. This library is already quite
fast, I think even that overhead would be preferable to the status quo,
which is obviously a security compromise for any application that uses this
function with unicode text coming from the user.
Not sure I express myself correctly, so I ...
confirm the bug exists
confirm the patch works
think this bug should be fixed and this is preferable to the status quo.
but I have no commit rights to the mercurial trunk. I am just a user of SDL libs.
If I had write access, I would choose Ignacio's patch.
Additionally, I just state the patch is not fully perfect from a "text rendering" point of view. But the relevant use-case is very very unlikely to happen. Writing a perfect patch would be of medium difficulty, but would highly make the code un-readable. Fell free to write a patch!
On 2015-05-30 08:52:50 +0000, Iris Morelle wrote:
SDL_ttf maintainers: any news on this?
On 2017-09-10 06:06:56 +0000, Sam Lantinga wrote:
It looks like this is fixed in the current SDL_ttf release. Please reopen this bug if that's not the case.
Thanks!
On 2017-09-10 12:06:03 +0000, Sylvain wrote:
Created attachment 2921
patch updated to latest SDL_ttf
I tried and this is still present!
Here's the same patch for latest SDL_ttf.
It does not solve the rendering, but prevent the invalid memory access.
I am re-opening this issue because the problem is half-solved. The memory issue is not there, but the rendering could be improved.
In fact the test-case is misleading because the first char is a space. So you don't see where it renders :)
If you re-use the same test-case, just change:
const int sample_size = 160;
the broken string, from:
" \315\241B", /* ' ' + U+0361 + 'B' */
to
".\315\241BC", /* '.' + U+0361 + 'B' + 'C' */
You'll see the dot '.' is shift by the negative minx offset of the second character.
Uploading two images I saved with "SDL_SaveBMP(textsurf, title);"
On 2018-10-19 20:32:22 +0000, Sylvain wrote:
Created attachment 3386
image current
This is the head version of SDL_ttf. The dot is a little bit shifted on the left.
On 2018-10-19 20:34:01 +0000, Sylvain wrote:
Created attachment 3387
image with new patch
This is the image with a new patch, that predicts the minx shift as xstart.
On 2018-10-19 20:35:55 +0000, Sylvain wrote:
In the new image, the dot is more centred.
Of course, this is a very small shift. One could say it doesn't happen in real text rendering but it does.
On 2018-10-19 20:37:24 +0000, Ozkan Sezer wrote:
(In reply to Sylvain from comment # 14)
Created attachment 3387 [details]
image with new patch
This is the image with a new patch, that predicts the minx shift as xstart.
Where is the new patch?
On 2018-10-19 20:40:02 +0000, Sylvain wrote:
Created attachment 3388
patch
Here's the patch.
It re-use the value minx computed by TTF_SizeUTF8, as an initial value of xstart. Then the negative bound checking is not necessary anymore.
(TTF_SizeUTF8() is a public API, so we can't change its parameter).
On 2018-10-19 20:42:18 +0000, Sylvain wrote:
The patch is somehow similar to what I propose for bug 4266.
Except one is providing a padding for minx/line, and the other for ymin.
On 2018-10-19 20:46:42 +0000, Ozkan Sezer wrote:
Haven't read the patch thoroughly along with the associtated
code yet, but is using the static variable there really truly
safe?
On 2018-10-19 21:03:54 +0000, Sylvain wrote:
TTF_SizeUTF8 is called to get the bounding box each time before rendering, so the static variable is always initialized.
... another way would be to have an internal TTF_Size to have the parameter returned. And that could be used by public TTF_Size*. Indeed probably cleaner.
For bug 4266, that would had another parameter too. TTF_initLineMectrics, and TTF_DrawLine*. That's more code to just transmit variable, but that's feasible.
I can update the patch next week.
On 2018-10-19 21:20:52 +0000, Ozkan Sezer wrote:
I tested the patch for the purpose of seeing whether the original
invalid reads still stay fixed: the default / 2.0 branch seems to
behave. But applying the new patch to the SDL-1.2 branch (which
has Sam's fix backported) brings back the invalid reads in there.
(Just a FYI note.)
On 2018-10-19 22:04:12 +0000, Ozkan Sezer wrote:
(In reply to Ozkan Sezer from comment # 21)
[...] But applying the new patch to the SDL-1.2 branch (which
has Sam's fix backported) brings back the invalid reads in there.
(Just a FYI note.)
Apologies, that was wrong: I missed applying one hunk of your patch
to TTF_SizeUNICODE(), so it is good in SDL-1.2 branch too.
TTF_SizeUTF8 is called to get the bounding box each time before
rendering, so the static variable is always initialized.
I mean, can TTF_SizeUTF8() not be called concurrently for different
fonts?
On 2018-10-20 20:07:09 +0000, Sylvain wrote:
Created attachment 3389
patch
SDL_ttf is not thread safe anyway, so there is not concurrency. Also because TTF_Size() is always called at the begining of the TTF_Render() functions. But the Wrapped function fix was probably wrong.
And the static variable would make SDL_ttf even more not thread safe.
So here's the new patch.
It adds TTF_SizeUTF8_Internal() and retrieve the xstart value. So no more static variable and the public API hasn't changed.
I have also tested the Wrapped function with a few broken case.
TTF_SizeUTF8_Internal needs to be called for each line to get the xstart value.
On 2018-10-23 10:37:07 +0000, Ozkan Sezer wrote:
Created attachment 3393
patch (for SDL-1.2 branch)
So here's the new patch.
Tested this v2 patch, seems to work OK. Attached a backport of it
for the SDL-1.2 branch.
On 2018-10-23 10:52:46 +0000, Sylvain wrote:
There is also a reproduced issue in # 4266 and I have a similar patch for it, you may also want for SDL-1.2
On 2018-10-23 11:04:02 +0000, Ozkan Sezer wrote:
(In reply to Sylvain from comment # 25)
There is also a reproduced issue in # 4266 and I have a similar patch for it,
you may also want for SDL-1.2
This bug report was migrated from our old Bugzilla tracker.
These attachments are available in the static archive:
Patch against SDL_ttf @ f28f7b0f72ae (SDL_ttf_hg.patch, text/plain, 2014-10-10 07:23:50 +0000, 1753 bytes)patch (patch_bug_2749_ttf_minx.diff, text/plain, 2018-10-19 20:40:02 +0000, 3626 bytes)Reported in version: 2.0.12
Reported for operating system, platform: Linux, x86_64
Comments on the original bug report:
On 2014-10-10 07:23:50 +0000, Iris Morelle wrote:
On 2014-10-10 07:24:41 +0000, Iris Morelle wrote:
On 2014-10-10 07:25:17 +0000, Iris Morelle wrote:
On 2014-10-14 19:01:44 +0000, Sylvain wrote:
On 2014-10-20 01:03:51 +0000, Iris Morelle wrote:
On 2014-10-24 13:14:38 +0000, Sylvain wrote:
On 2014-10-25 02:34:31 +0000, Chris Beck wrote:
On 2014-10-31 11:02:34 +0000, Sylvain wrote:
On 2015-05-30 08:52:50 +0000, Iris Morelle wrote:
On 2017-09-10 06:06:56 +0000, Sam Lantinga wrote:
On 2017-09-10 12:06:03 +0000, Sylvain wrote:
On 2017-09-10 17:21:49 +0000, Sam Lantinga wrote:
On 2018-10-19 20:30:12 +0000, Sylvain wrote:
On 2018-10-19 20:32:22 +0000, Sylvain wrote:
On 2018-10-19 20:34:01 +0000, Sylvain wrote:
On 2018-10-19 20:35:55 +0000, Sylvain wrote:
On 2018-10-19 20:37:24 +0000, Ozkan Sezer wrote:
On 2018-10-19 20:40:02 +0000, Sylvain wrote:
On 2018-10-19 20:42:18 +0000, Sylvain wrote:
On 2018-10-19 20:46:42 +0000, Ozkan Sezer wrote:
On 2018-10-19 21:03:54 +0000, Sylvain wrote:
On 2018-10-19 21:20:52 +0000, Ozkan Sezer wrote:
On 2018-10-19 22:04:12 +0000, Ozkan Sezer wrote:
On 2018-10-20 20:07:09 +0000, Sylvain wrote:
On 2018-10-23 10:37:07 +0000, Ozkan Sezer wrote:
On 2018-10-23 10:52:46 +0000, Sylvain wrote:
On 2018-10-23 11:04:02 +0000, Ozkan Sezer wrote:
On 2018-10-24 17:01:06 +0000, Ozkan Sezer wrote:
The text was updated successfully, but these errors were encountered: