Bug 866989 - double free in _nc_scroll_optimize()
double free in _nc_scroll_optimize()
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: ncurses (Show other bugs)
18
x86_64 Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Miroslav Lichvar
Fedora Extras Quality Assurance
abrt_hash:df4a62de932c6ccc3f50eab914c...
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-16 09:41 EDT by V'Ger Roby
Modified: 2012-12-20 10:29 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-12-20 10:29:33 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
File: core_backtrace (1.38 KB, text/plain)
2012-10-16 09:41 EDT, V'Ger Roby
no flags Details
File: environ (3.00 KB, text/plain)
2012-10-16 09:41 EDT, V'Ger Roby
no flags Details
File: backtrace (9.40 KB, text/plain)
2012-10-16 09:41 EDT, V'Ger Roby
no flags Details
File: limits (1.29 KB, text/plain)
2012-10-16 09:41 EDT, V'Ger Roby
no flags Details
File: cgroup (130 bytes, text/plain)
2012-10-16 09:41 EDT, V'Ger Roby
no flags Details
File: maps (3.24 KB, text/plain)
2012-10-16 09:41 EDT, V'Ger Roby
no flags Details
File: dso_list (771 bytes, text/plain)
2012-10-16 09:41 EDT, V'Ger Roby
no flags Details
File: proc_pid_status (926 bytes, text/plain)
2012-10-16 09:41 EDT, V'Ger Roby
no flags Details
File: var_log_messages (221 bytes, text/plain)
2012-10-16 09:41 EDT, V'Ger Roby
no flags Details
File: open_fds (138 bytes, text/plain)
2012-10-16 09:41 EDT, V'Ger Roby
no flags Details
File: build_ids (287 bytes, text/plain)
2012-10-16 09:41 EDT, V'Ger Roby
no flags Details
last diff, with checkin comment (1.52 KB, patch)
2012-10-16 20:57 EDT, Thomas E. Dickey
no flags Details | Diff

  None (edit)
Description V'Ger Roby 2012-10-16 09:41:24 EDT
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 09:41:28 EDT
Created attachment 628163 [details]
File: core_backtrace
Comment 2 V'Ger Roby 2012-10-16 09:41:30 EDT
Created attachment 628164 [details]
File: environ
Comment 3 V'Ger Roby 2012-10-16 09:41:34 EDT
Created attachment 628165 [details]
File: backtrace
Comment 4 V'Ger Roby 2012-10-16 09:41:36 EDT
Created attachment 628166 [details]
File: limits
Comment 5 V'Ger Roby 2012-10-16 09:41:38 EDT
Created attachment 628167 [details]
File: cgroup
Comment 6 V'Ger Roby 2012-10-16 09:41:41 EDT
Created attachment 628168 [details]
File: maps
Comment 7 V'Ger Roby 2012-10-16 09:41:43 EDT
Created attachment 628169 [details]
File: dso_list
Comment 8 V'Ger Roby 2012-10-16 09:41:45 EDT
Created attachment 628170 [details]
File: proc_pid_status
Comment 9 V'Ger Roby 2012-10-16 09:41:47 EDT
Created attachment 628171 [details]
File: var_log_messages
Comment 10 V'Ger Roby 2012-10-16 09:41:49 EDT
Created attachment 628172 [details]
File: open_fds
Comment 11 V'Ger Roby 2012-10-16 09:41:52 EDT
Created attachment 628173 [details]
File: build_ids
Comment 12 Kamil Dudka 2012-10-16 11:13:02 EDT
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 11:26:48 EDT
The ncurses version from the dso_list is is 5.9-20121013.
Comment 14 V'Ger Roby 2012-10-16 12:20:16 EDT
Should I do something? Is it solved?
Comment 15 Miroslav Lichvar 2012-10-16 12:41:01 EDT
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-16 20:57:27 EDT
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 03:26:14 EDT
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 04:53:46 EDT
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 05:00:56 EDT
...but I agree that removing the line is the fix needed :-)
Comment 20 V'Ger Roby 2012-10-17 05:31:17 EDT
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 05:35:56 EDT
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 05:51:16 EDT
...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 06:10:45 EDT
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 06:43:27 EDT
See comment #21 (long day -> increased time to wake up).
Comment 25 V'Ger Roby 2012-10-17 06:53:12 EDT
Oh, ok.. thank you for your patience..
Comment 26 Fedora Update System 2012-10-18 05:52:08 EDT
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 11:25:53 EDT
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 18:02:18 EDT
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-18 20:28:00 EDT
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 10:29:36 EST
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.

Note You need to log in before you can comment on or make changes to this bug.