| Summary: | Invalid memory read & write by TTF_RenderUTF8* functions with specific input | ||
|---|---|---|---|
| Product: | SDL_ttf | Reporter: | Iris Morelle <shadowm> |
| Component: | misc | Assignee: | Sam Lantinga <slouken> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | major | ||
| Priority: | P2 | CC: | render787, sezeroz, sylvain.becker |
| Version: | 2.0.12 | ||
| Hardware: | x86_64 | ||
| OS: | Linux | ||
| Attachments: |
Patch against SDL_ttf @ f28f7b0f72ae
Standalone test case Valgrind memcheck output without the patch applied patch updated to latest SDL_ttf image current image with new patch patch patch patch (for SDL-1.2 branch) |
||
|
Description
Iris Morelle
2014-10-10 07:23:50 UTC
Created attachment 1894 [details]
Standalone test case
Created attachment 1895 [details]
Valgrind memcheck output without the patch applied
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. First, it looks to me similar with this (solved) ticket: https://bugzilla.libsdl.org/show_bug.cgi?id=2470 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. 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. 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. "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. (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! SDL_ttf maintainers: any news on this? It looks like this is fixed in the current SDL_ttf release. Please reopen this bug if that's not the case. Thanks! Created attachment 2921 [details]
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.
Thanks for the excellent test case, this is now fixed: https://hg.libsdl.org/SDL_ttf/rev/b9d515ee0aaf 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);" Created attachment 3386 [details]
image current
This is the head version of SDL_ttf. The dot is a little bit shifted on the left.
Created attachment 3387 [details]
image with new patch
This is the image with a new patch, that predicts the minx shift as xstart.
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. (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? Created attachment 3388 [details]
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).
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. Haven't read the patch thoroughly along with the associtated code yet, but is using the static variable there really truly safe? 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. 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.) (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? Created attachment 3389 [details]
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.
Created attachment 3393 [details] 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. There is also a reproduced issue in #4266 and I have a similar patch for it, you may also want for SDL-1.2 (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 OK. Sam: any objections? The patch is in, and backported to SDL-1.2 branch too. Closing. https://hg.libsdl.org/SDL_ttf/rev/723fbb253f39 https://hg.libsdl.org/SDL_ttf/rev/e6128bef6a13 |