Bug 866989

Summary: double free in _nc_scroll_optimize()
Product: [Fedora] Fedora Reporter: V'Ger Roby <vger79r>
Component: ncursesAssignee: Miroslav Lichvar <mlichvar>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: 18CC: dickey, dwmw2, jaswinder, kdudka, mlichvar
Target Milestone: ---   
Target Release: ---   
Hardware: x86_64   
OS: Unspecified   
Whiteboard: abrt_hash:df4a62de932c6ccc3f50eab914c625932c12732f
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-12-20 15:29:33 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
File: core_backtrace
none
File: environ
none
File: backtrace
none
File: limits
none
File: cgroup
none
File: maps
none
File: dso_list
none
File: proc_pid_status
none
File: var_log_messages
none
File: open_fds
none
File: build_ids
none
last diff, with checkin comment none

Description V'Ger Roby 2012-10-16 13:41:24 UTC
Version-Release number of selected component:
nano-2.3.1-4.fc18

Additional info:
libreport version: 2.0.16
abrt_version:   2.0.15
backtrace_rating: 4
cmdline:        nano Git/FlatZinc/NFloat.Literal.cpp
crash_function: _nc_scroll_optimize
kernel:         3.6.1-1.fc18.x86_64

truncated backtrace:
:Thread no. 1 (7 frames)
: #5 _nc_scroll_optimize at ../../ncurses/tty/hardscroll.c:208
: #6 doupdate at ../../ncurses/tty/tty_update.c:927
: #7 get_key_buffer at winio.c:122
: #8 get_input at winio.c:259
: #9 parse_kbinput at winio.c:342
: #10 get_kbinput at winio.c:314
: #11 do_input at nano.c:1537

Comment 1 V'Ger Roby 2012-10-16 13:41:28 UTC
Created attachment 628163 [details]
File: core_backtrace

Comment 2 V'Ger Roby 2012-10-16 13:41:30 UTC
Created attachment 628164 [details]
File: environ

Comment 3 V'Ger Roby 2012-10-16 13:41:34 UTC
Created attachment 628165 [details]
File: backtrace

Comment 4 V'Ger Roby 2012-10-16 13:41:36 UTC
Created attachment 628166 [details]
File: limits

Comment 5 V'Ger Roby 2012-10-16 13:41:38 UTC
Created attachment 628167 [details]
File: cgroup

Comment 6 V'Ger Roby 2012-10-16 13:41:41 UTC
Created attachment 628168 [details]
File: maps

Comment 7 V'Ger Roby 2012-10-16 13:41:43 UTC
Created attachment 628169 [details]
File: dso_list

Comment 8 V'Ger Roby 2012-10-16 13:41:45 UTC
Created attachment 628170 [details]
File: proc_pid_status

Comment 9 V'Ger Roby 2012-10-16 13:41:47 UTC
Created attachment 628171 [details]
File: var_log_messages

Comment 10 V'Ger Roby 2012-10-16 13:41:49 UTC
Created attachment 628172 [details]
File: open_fds

Comment 11 V'Ger Roby 2012-10-16 13:41:52 UTC
Created attachment 628173 [details]
File: build_ids

Comment 12 Kamil Dudka 2012-10-16 15:13:02 UTC
This looks like a bug in ncurses to me:

  int *new_oldnums = typeRealloc(int,
               (size_t) need_lines,
               oldnums(SP_PARM));
  if (!new_oldnums)
      return;
  FreeIfNeeded(oldnums(SP_PARM));

If a pointer is given to realloc(), you should not free() the pointer afterwards.

Comment 13 Miroslav Lichvar 2012-10-16 15:26:48 UTC
The ncurses version from the dso_list is is 5.9-20121013.

Comment 14 V'Ger Roby 2012-10-16 16:20:16 UTC
Should I do something? Is it solved?

Comment 15 Miroslav Lichvar 2012-10-16 16:41:01 UTC
No, I've just CCed the upstream maintainer. He may know if there were changes around this code recently.

Comment 16 Thomas E. Dickey 2012-10-17 00:57:27 UTC
Created attachment 628479 [details]
last diff, with checkin comment

From this discussion, this seems to be an incorrect bug-fix :-)

Comment 17 V'Ger Roby 2012-10-17 07:26:14 UTC
Ok.. ..let me verify if I got it right:
1. It reallocs a chunk of memory, which may or may not move the original block of data depending on the segmentation level of the heap.
2. Tests the returned pointer (Here i don't really get why he uses !new_oldnums. It's clear that if new_oldnums == 0 then !new_oldnums == 0xFF. But when new_oldnums == RandomNumber, the "complement" given by ! negation operator, does really return 0?)
3. Frees the memory previously associated to the pointer. Here two things may happen:
A- the original chunk of memory wasn't moved for memory issues, so he is freeing the newly reallocated chunk of memory
B- the original chunk of memory was moved, so he is freeing a piece of memory which has already been freed automatically by realloc.

In both cases it's an error, assuming those functions behave as standard free/realloc. I've tried to search for typeRealloc and FreeIfNeeded, but found nothing.. do you know where i can check their implementation? Ncurses?

The solution should be to simply remove line
+	FreeIfNeeded(oldnums(SP_PARM));
which is not needed at all. Though i don't really understand if it's safe to return the memory "not resized" in case typeRealloc fails, as he does. :-/

By the way, how did you get that interesting diff? :-)

Comment 18 Thomas E. Dickey 2012-10-17 08:53:46 UTC
hmm - no - the problem that I see is not checking to ensure that
the old/new pointers are really different before doing the free
(as well as looking for other pitfalls of the same type).

The "interesting diff" is a chunk from rcshist.

Comment 19 Thomas E. Dickey 2012-10-17 09:00:56 UTC
...but I agree that removing the line is the fix needed :-)

Comment 20 V'Ger Roby 2012-10-17 09:31:17 UTC
well.. I don't get why you think the problem is only checking that the pointers are different..

..the pointers are different only if the chunk of memory is moved at another address for memory management issues, so the original memory address should be already "freed" automatically by realloc.

But I'm not an expert. :-/

Comment 21 Thomas E. Dickey 2012-10-17 09:35:56 UTC
On the other hand, I had a long day, and today's similar :-)

I'll put out a patch for this, and review the other realloc's
to fix the (already known) issue that using the result from
realloc requires checking it before keeping it.

Comment 22 Thomas E. Dickey 2012-10-17 09:51:16 UTC
...if realloc fails, the original memory is supposed to be intact.
I had a report on this a while back, and fixed the existing cases.
There are some new ones that I see.  I'll put out the short fix this
morning, and work through the others (perhaps tomorrow - won't be
around this weekend).

Comment 23 V'Ger Roby 2012-10-17 10:10:45 UTC
Yep, I totally agree with you on this. But still don't get why you suggested "the problem that I see is not checking to ensure that the old/new pointers are really different before doing the free".

A) realloc fails: memory untouched, returns NULL: the original pointer should not immediately overwritten to not cause a memory leakage, nor freed;
B) realloc is successfull, but doesn't move the object in the heap (just adjusts its end): returns the same address passed as a parameter, hence freeing it is an error;
C) realloc is successful, and moves the object in the heap: the new address must be stored in the old variable pointer. The old pointer must not be freed, since realloc has already done it.


So, even if the pointers are indeed different, you should never free anything at all. No?

Comment 24 Thomas E. Dickey 2012-10-17 10:43:27 UTC
See comment #21 (long day -> increased time to wake up).

Comment 25 V'Ger Roby 2012-10-17 10:53:12 UTC
Oh, ok.. thank you for your patience..

Comment 26 Fedora Update System 2012-10-18 09:52:08 UTC
ncurses-5.9-7.20121017.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/ncurses-5.9-7.20121017.fc18

Comment 27 Fedora Update System 2012-10-18 15:25:53 UTC
Package ncurses-5.9-7.20121017.fc18:
* should fix your issue,
* was pushed to the Fedora 18 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing ncurses-5.9-7.20121017.fc18'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2012-16392/ncurses-5.9-7.20121017.fc18
then log in and leave karma (feedback).

Comment 28 V'Ger Roby 2012-10-18 22:02:18 UTC
That's amazing.. but I've got a problem.

This morning was released an update to rhythmbox, right after which I was able to launch it again.. so note that by leaving a karma I'm saying that that update + your bugfix (which was proven to exist) have solved the launch problem.

I've seen that the IM plugin does not instead still work. Do you think that it's better to contact the plugin maker, wait some time for library stabilization or do something else?

Comment 29 Thomas E. Dickey 2012-10-19 00:28:00 UTC
I'm not sure about the IM plugin.  If I'm finding some particular
problem with a program, I do check if there's similar reports
(and for instance look to see if it's being handled).

Comment 30 Fedora Update System 2012-12-20 15:29:36 UTC
ncurses-5.9-7.20121017.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.