Bug 40577 - Missing bound checking in FreeSelectionProperty() from Selection.c
Summary: Missing bound checking in FreeSelectionProperty() from Selection.c
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Lib/Xt (show other bugs)
Version: git
Hardware: All All
: medium normal
Assignee: Xorg Project Team
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2011-09-02 05:08 UTC by Olivier Fourdan
Modified: 2011-10-03 12:56 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Simple reproducer program (1.17 KB, text/plain)
2011-09-02 05:08 UTC, Olivier Fourdan
no flags Details
Proposed patch (946 bytes, patch)
2011-09-02 05:10 UTC, Olivier Fourdan
no flags Details | Splinter Review
Proposed patch (658 bytes, patch)
2011-09-02 05:13 UTC, Olivier Fourdan
no flags Details | Splinter Review

Description Olivier Fourdan 2011-09-02 05:08:42 UTC
Created attachment 50840 [details]
Simple reproducer program

Description

FreeSelectionProperty() does not check for the count of items in array and
relies on a NULL terminated list, which can cause libXt to crash if
FreeSelectionProperty() follows a call to GetSelectionProperty() which
reallocates the array.

Version-Release number of selected component (if applicable):

All including current git

How reproducible:

Randomly, requires a selection in gtk+ version < 2.16.6 

Steps to Reproduce:

  1. Use a system with gtk+ version < 2.16.6 (see additional info as to why)
  2. Download and save the attached reproducer in C "selectiontest.c"
  3. Build the reproducer with:

   $ gcc -o selectiontest -Wall -Wextra -lXm -lXt -lX11 selectiontest.c

  4. Run the reproducer

   $ ./selectiontest

  5. Select some text with the mouse in another gtk application such as gedit
     or gnome-terminal

  6. Click on the button "Click here" in the reproducer while the selection
     is still active in the gtk+ application

Actual results:

  The reproducer crashes in Xt code FreeSelectionProperty()

Expected results:

  No crash

Additional info:

Valgrind shows the following error:

  ==18507== Invalid read of size 8
  ==18507==    at 0x532F6BE: FreeSelectionProperty (Selection.c:251)
  ==18507==    by 0x532FD90: HandleSelectionReplies (Selection.c:1400)
  ==18507==    by 0x531C1C8: XtDispatchEventToWidget (Event.c:901)
  ==18507==    by 0x531C563: _XtDefaultDispatcher (Event.c:1344)
  ==18507==    by 0x531C853: XtDispatchEvent (Event.c:1423)
  ==18507==    by 0x531CCFA: XtAppMainLoop (Event.c:1560)
  ==18507==    by 0x400A40: main (in /home/ofourdan/selectiontest)
  ==18507==  Address 0x85d7e70 is 0 bytes after a block of size 64 alloc'd
  ==18507==    at 0x4C26582: realloc (vg_replace_malloc.c:525)
  ==18507==    by 0x530D532: XtRealloc (Alloc.c:189)
  ==18507==    by 0x532E550: GetSelectionProperty (Selection.c:227)
  ==18507==    by 0x5330300: GetSelectionValues.part.4 (Selection.c:1686)
  ==18507==    by 0x5331FF1: XtGetSelectionValues (Selection.c:1636)
  ==18507==    by 0x400989: cb_clicked (in /home/ofourdan/selectiontest)
  ==18507==    by 0x530E0AF: XtCallCallbackList (Callback.c:625)
  ==18507==    by 0x4F13C76: ActivateCommon (PushB.c:2247)
  ==18507==    by 0x5342EAC: HandleActions (TMstate.c:645)
  ==18507==    by 0x5343CB6: _XtTranslateEvent (TMstate.c:1071)
  ==18507==    by 0x531C052: XtDispatchEventToWidget (Event.c:906)
  ==18507==    by 0x531C795: _XtDefaultDispatcher (Event.c:1367)

The original problem relies in gtk+ which creates an atom for "NONE", this is GNOME bug 580511:

    https://bugzilla.gnome.org/show_bug.cgi?id=580511

Which was fixed with this git commit 6aa63385:
   
    http://git.gnome.org/browse/gtk+/commit/gdk/x11/gdkproperty-x11.c?id=6aa63385789fd28e61eb4de05312e2f1316cf6d0

However, even if Xt is not the source of the problem, it should not crash because of another (broken) application.

Proposed patch attached, which checks for the "propCount" value in FreeSelectionProperty()
Comment 1 Olivier Fourdan 2011-09-02 05:10:15 UTC
Created attachment 50841 [details] [review]
Proposed patch

Check against propCount while reading the array.
Comment 2 Olivier Fourdan 2011-09-02 05:13:37 UTC
Created attachment 50842 [details] [review]
Proposed patch

Same patch, without the NL
Comment 3 Alan Coopersmith 2011-10-03 12:56:46 UTC
Pushed to git master:
http://cgit.freedesktop.org/xorg/lib/libXt/commit/?id=9347b890ba24db41c7cb6c6e76564e4896bc8cac

Thanks for the report & fix.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.