| Summary: | 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. | ||
|---|---|---|---|
| Product: | xorg | Reporter: | Sachin Kumar Gupta <s.k.gupta> |
| Component: | Lib/ICE | Assignee: | Xorg Project Team <xorg-team> |
| Status: | RESOLVED MOVED | QA Contact: | Xorg Project Team <xorg-team> |
| Severity: | major | ||
| Priority: | medium | CC: | caolanm, LibreOffice, s.k.gupta |
| Version: | unspecified | Keywords: | patch |
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: | |||
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. Alan, thanks for insight into it. I will be making a new patch for the it, incorporating all your suggestions soon. (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. Created attachment 105734 [details] [review] New Patch for the raised bug, incorporating the changes suggested by Alan Reminder for patch review. 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. Created attachment 106497 [details] [review] Patch for the reported bug formatted in accordance to x-org guidelines 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. 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. 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. 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. (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. -- 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.
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".