Bug 83235 - Accessing freed memory in LibICE version 1.0.9 in file "process.c" at line 1177 ,as the relevant pointer is not assigned NULL after freeing.
Summary: Accessing freed memory in LibICE version 1.0.9 in file "process.c" at line 11...
Status: RESOLVED MOVED
Alias: None
Product: xorg
Classification: Unclassified
Component: Lib/ICE (show other bugs)
Version: unspecified
Hardware: All All
: medium major
Assignee: Xorg Project Team
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2014-08-29 08:52 UTC by Sachin Kumar Gupta
Modified: 2018-08-10 20:20 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:

Attachments
Patch for the reported bug. (1.52 KB, text/plain)
2014-08-29 08:52 UTC, Sachin Kumar Gupta 		no flags Details
New Patch for the raised bug, incorporating the changes suggested by Alan (16.45 KB, patch)
2014-09-04 12:00 UTC, Sachin Kumar Gupta 		no flags Details | Splinter Review
Patch for the reported bug formatted in accordance to x-org guidelines (23.27 KB, patch)
2014-09-18 10:53 UTC, Sachin Kumar Gupta 		no flags Details | Splinter Review
Show Obsolete (2) View All

Description Sachin Kumar Gupta 2014-08-29 08:52:57 UTC 
Created attachment 105420 [details]
Patch for the reported bug.

Component: LibICE
Version : 1.0.9

File where error is: src/process.c
Function where error is: ProcessAuthRequired
Line of Error: 1177


In reference to code mentioned below:

Here the "iceConn->connect_to_you" in "if conditional" may have been freed before by " IceReadCompleteMessage" which is a macro by internally calling "_IceRead" which calls "_IceFreeConnection", which frees "iceConn->connect_to_you" but does not assigns NULL to it , so even if "iceConn->connect_to_you" has been freed, the if conditional will evaluate TRUE as "iceConn->connect_to_you" has not been assigned NULL and in this case, it is being dereferenced later as:

 iceConn->connect_to_you->auth_active = 1;


Code where error exists(LibICE v1.0.9 code):


   if (iceConn->connect_to_you)
    {
  if ((int) message->authIndex >= _IceAuthCount)
  {
      _IceConnectionError *errorReply =
          &(((_IceReply *) (replyWait->reply))->connection_error);

      const char *tempstr
    = "Received bad authIndex in the AuthRequired message";
      char errIndex = (int) message->authIndex;

      errorString = strdup(tempstr);

      errorReply->type = ICE_CONNECTION_ERROR;
      errorReply->error_message = errorString;

      _IceErrorBadValue (iceConn, 0,
    ICE_AuthRequired, 2, 1, &errIndex);

      IceDisposeCompleteMessage (iceConn, authData);
      return (1);
  }
  else
  {
      authProc = _IcePoAuthProcs[message->authIndex];

      iceConn->connect_to_you->auth_active = 1;
  }
    }

File where fix is to be made:"src/shutdown.c"
Function where fix is required: _IceFreeConnection

LibICE version 1.0.9 code :

    if (iceConn->connect_to_you)
  free (iceConn->connect_to_you);

Recommended Code:

  if (iceConn->connect_to_you)
    {
        free (iceConn->connect_to_you);
        iceConn->connect_to_you = NULL;
    }

I am attaching a patch for the same "shutdown.patch".
Comment 1 Alan Coopersmith 2014-08-29 23:41:57 UTC 
Setting the pointer to NULL in _IceFreeConnection is pointless, since that
only affects the local copy in that function, and when you exit the function
immediately afterwards, you're back in the caller with a non-NULL copy.

For this to work, the macro needs to check if _IceRead returns 0, and
if so, set the _iceConn passed to _IceRead to be NULL.

Of course, that just moves the segfault around until you fix the other
macros to check for _iceConn being NULL before dereferencing it.
Comment 2 Sachin Kumar Gupta 2014-09-02 14:40:42 UTC 
Alan, thanks for insight into it. I will be making a new patch for the it, incorporating all your suggestions soon.
Comment 3 Sachin Kumar Gupta 2014-09-04 11:50:19 UTC 
(In reply to comment #1)


Hello Alan, I am attaching a new Patch "ICE_iceconn.patch", which incorporates all the points raised and suggestions given by you.

It incorporates changes in 
(a) 'ICEmsg.h' to fix  other macros to check for _iceConn being NULL before    dereferencing it.

(b) 'misc.c' where function _IceReadSkip has been modified to return 0 when _IceRead frees iceConn and 1 in other cases.

(c)'process.c' to check for _iceConn being NULL before    dereferencing it.

(d) 'protosetup.c' to handle the situation when in function 'IceProtocolSetup' the function call to 'IceProcessMessages' at line 196, which internally calls '_IceRead' which may free '_iceConn', leads to derefercing of already  freed iceConn.

File :prtotosetup.c
Line where error may occur:196  


ICE version 1.0.9 code:


    while (!gotReply && !ioErrorOccured)
    {
  ioErrorOccured = (IceProcessMessages (
      iceConn, &replyWait, &gotReply) == IceProcessMessagesIOError);

  if (ioErrorOccured)


Recommended Code:


    while (!gotReply && !ioErrorOccured)
    {
     msgStatus = IceProcessMessages ( iceConn, &replyWait, &gotReply);
     if (msgStatus == IceProcessMessagesConnectionClosed)
     {
       iceConn == NULL;
       strncpy (errorStringRet,
                   "Connection Closure occured doing Protocol Setup on connection",
                       errorLength);

       return (IceProtocolSetupFailure);
     }
     ioErrorOccured = (msgStatus == IceProcessMessagesIOError);

  if (ioErrorOccured)
  {


To see rest of the code changes kindly see the attached patch.
Comment 4 Sachin Kumar Gupta 2014-09-04 12:00:26 UTC 
Created attachment 105734 [details] [review]
New Patch for the raised bug, incorporating the changes suggested by Alan
Comment 5 Sachin Kumar Gupta 2014-09-17 04:54:38 UTC 
Reminder for patch review.
Comment 6 Alan Coopersmith 2014-09-17 05:04:25 UTC 
Patch review may go faster if you follow the instructions on:
http://wiki.x.org/wiki/Development/Documentation/SubmittingPatches/

But really, libICE has so little developer interest, patches may take a long
time to get reviewed no matter what.
Comment 7 Sachin Kumar Gupta 2014-09-18 10:53:33 UTC 
Created attachment 106497 [details] [review]
Patch for the reported bug formatted in accordance to x-org guidelines
Comment 8 Sachin Kumar Gupta 2014-09-18 11:01:05 UTC 
A new patch has been attached which follows the x-org guidelines. Kindly see to it and let me know it further changes or modifications are required.
Comment 9 Sachin Kumar Gupta 2014-09-21 19:04:04 UTC 
Hello Alan , i have attached a new patch in accordance to the x-org guidelines. You are requested to have a look at it, and pass on relevant suggestions,as and when you prefer.
Comment 10 Alan Coopersmith 2014-09-22 06:44:21 UTC 
Please stop adding me to bug reports.  I already get the xorg-team
mail, I don't need you requiring me to delete double the mail for 
every update.
Comment 11 Alan Coopersmith 2014-10-25 01:19:21 UTC 
Because of it's use in the macros in the public header, _IceReadSkip is
exported API of libICE, and thus can't be changed incompatibly like that
without breaking compatibility with existing callers.
Comment 12 Sachin Kumar Gupta 2014-10-28 09:11:20 UTC 
(In reply to Alan Coopersmith from comment #11)
> Because of it's use in the macros in the public header, _IceReadSkip is
> exported API of libICE, and thus can't be changed incompatibly like that
> without breaking compatibility with existing callers.

@Alan : So what do you suggest.. should i make a new patch where there is no change in return type of _ICEReadSkip how ever dereferencing of NULL iceConn in locations of _ICEreadSkip is avoided and keep the other modifications as it is such as the changes in macros etc.
Comment 13 GitLab Migration User 2018-08-10 20:20:03 UTC 
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/xorg/lib/libice/issues/3.

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.