bugThe FreeType Project - Bugs: bug #43682, FreeType 2.5.3 multiple unchecked...

 
 

bug #43682: FreeType 2.5.3 multiple unchecked function calls returning FT_Error

Submitted by:  Mateusz Jurczyk <j00ru>
Submitted on:  Mon 24 Nov 2014 10:12:23 AM UTC  
 
Severity: 3 - NormalItem Group: Incorrect behaviour
Status: FixedPrivacy: Public
Assigned to: Werner LEMBERG <wl>Open/Closed: Closed
Planned Release: 2.5.4

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

Tue 25 Nov 2014 11:26:45 AM UTC, comment #2:

Wow, thanks for the prompt response and fixes! :-)

Mateusz Jurczyk <j00ru>
Project Member
Tue 25 Nov 2014 10:40:33 AM UTC, comment #1:

It was an excellent idea to check this, thanks! I've now reviewed the source code, which resulted in a series of commits, please have a look.

My philosophy is to add casts to `void' only for functions that can return non-trivial errors. `Trivial' errors are caused by invalid arguments to API functions, and they don't get a cast.

As far as I can see, none of the added error checkings are critical, but it's good to have them fixed, too.

Werner LEMBERG <wl>
Project AdministratorIn charge of this item.
Mon 24 Nov 2014 10:12:23 AM UTC, original submission:

The CFF-related heap corruption vulnerability reported at https://savannah.nongnu.org/bugs/?43658 was caused by lack of function exit code validation and propagation (see the fix at http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=5f201ab5c24cb69bc96b724fd66e739928d6c5e2).

This might imply that the project code base might have more instances of function calls with unchecked return values, potentially denoting error conditions. Continuing execution in the caller function could then lead to operating on inconsistent memory state (based on the false assumption that all callees have succeeded) and consequently to various memory errors, often security related.

Fortunately, FreeType has its own type for completion status: "FT_Error" defined in include/fttypes.h:

---
287: /*************************************************************************/
288: /* */
289: /* <Type> */
290: /* FT_Error */
291: /* */
292: /* <Description> */
293: /* The FreeType error code type. A value of~0 is always interpreted */
294: /* as a successful operation. */
295: /* */
296: typedef int FT_Error;
---

Thanks to this, it is relatively easy to use the gcc/clang compiler to point out all occurrences of unchecked FT_Error return values by replacing line 296 with:

296: #define FT_Error int _attribute_((warn_unused_result))

and adding the -Wno-attributes switch (in case of gcc):

$ CFLAGS=-Wno-attributes ./configure

When we then run "make", gcc will print out warnings of the following format:

---
freetype2/src/base/ftbitmap.c: In function 'FT_Bitmap_Embolden':
freetype2/src/base/ftbitmap.c:303:23: warning: ignoring return value of 'FT_Bitmap_Done', declared with attribute warn_unused_result [-Wunused-result]
FT_Bitmap_Done( library, bitmap );
^
freetype2/src/base/ftglyph.c: In function 'ft_bitmap_glyph_done':
freetype2/src/base/ftglyph.c:117:19: warning: ignoring return value of 'FT_Bitmap_Done', declared with attribute warn_unused_result [-Wunused-result]
FT_Bitmap_Done( library, &glyph->bitmap );
---

Logs indicate that across the entire codebase (from git master as of today) reachable by the compiler on a GNU/Linux system, there are a total of 100 instances of unchecked FT_Error:

$ make 2>&1 | grep "ignoring return value" | wc -l
100

A complete list of the warnings is attached. As "FT_Error" is designed specifically to pass error codes around, we assume it should be generally checked and propagated for all functions which return it. Of course not all unchecked function calls result in security issues or even bugs, but properly handling exit codes is a good practice and a way to prevent bugs in the future (and some of them in fact can lead to security vulnerabilities, as Savannah bug #43658 showed).

Please let me know about your thoughts on this. It probably doesn't make sense to fix all those warnings (some of the function invocations are of the "(void)function()" form, indicating that ignoring the return value is intentional and safe), but just looking through them and fixing legitimate bugs should greatly improve the security posture of the code.

While I am not very familiar with FreeType as a developer, I can probably help investigating the warnings / developing patches, if need be. I'm looking forward to your comments.

Mateusz Jurczyk <j00ru>
Project Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #32535:  unchecked_ft_error.txt added by j00ru (15KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by wl (Posted a comment)
  • -unavailable- added by j00ru (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 25 Nov 2014 10:40:33 AM UTCwlStatusNone=>Fixed
      PrivacyPrivate=>Public
      Assigned toNone=>wl
      Open/ClosedOpen=>Closed
      Planned ReleaseNone=>2.5.4
    Mon 24 Nov 2014 10:12:23 AM UTCj00ruAttached File-=>Added unchecked_ft_error.txt, #32535

    Back to the top


    Powered by Savane 3.1-cleanup1