| | [ Return to Bugs & Features | Post Text | Post File | SVN ⇄ GIT | Prev | Next ]
STR #3231
| Application: | FLTK Library |
|---|
| Status: | 4 - Pending |
|---|
| Priority: | 4 - High, e.g. key functionality not working |
|---|
| Scope: | 3 - Applies to all machines and operating systems |
|---|
| Subsystem: | Example Programs |
|---|
| Summary: | editor.cxx: changed_cb() inducing reading uninitialized memory |
|---|
| Version: | 1.4-feature |
|---|
| Created By: | greg.ercolano |
|---|
| Assigned To: | greg.ercolano |
|---|
| Fix Version: | Unassigned (SVN: v12000) |
|---|
| Update Notification: | |
|---|
Trouble Report Files:
[ Post File ]
Trouble Report Comments:
[ Post Text ]
| |
| #1 | greg.ercolano
23:03 May 25, 2015 |
| While valgrind'ing the test/editor example, getting many errors
about reading uninitialized memory during file load().
Replication:
1. Run the test/editor with valgrind, e.g. ( cd test; valgrind ./editor)
2. Type 'asdf<CR>asdf<CR>asdf'
3. Choose File -> Load to load a new file, do not save current editor contents
4. Trips this issue that accesses invalid memory contents
The cause appears to be the editor's changed_cb() forcing calls to
show_insert_position() during the file load() operation, which is causing
the old cursor position to point past the end of the buffer, causing the
editor internals to consider memory no longer valid.
Here's the current contents of the test/editor.cxx's changed_cb() code:
----
void changed_cb(int, int nInserted, int nDeleted,int, const char*, void* v) {
if ((nInserted || nDeleted) && !loading) changed = 1;
EditorWindow *w = (EditorWindow *)v;
set_title(w);
if (loading) w->editor->show_insert_position(); // <-- THIS CAUSING THE TROUBLE
}
----
First, I wonder what the purpose of this is; why should the changed_cb() be
moving around the scroll position? And why do this only while the file is 'loading'?
But still, this probably shouldn't cause the editor to access memory it shouldn't.
Here's the valgrind output of the above replication with svn r10736:
==20970== Invalid read of size 1
==20970== at 0x426642: Fl_Text_Display::handle_vline(int, int, int, int, int, int, int, int, int) const (Fl_Text_Display.cxx:1937)
==20970== by 0x424A89: Fl_Text_Display::position_to_xy(int, int*, int*) const (Fl_Text_Display.cxx:1025)
==20970== by 0x424F11: Fl_Text_Display::display_insert() (Fl_Text_Display.cxx:1172)
==20970== by 0x423BA4: Fl_Text_Display::resize(int, int, int, int) (Fl_Text_Display.cxx:622)
==20970== by 0x425084: Fl_Text_Display::show_insert_position() (Fl_Text_Display.cxx:1199)
==20970== by 0x406AAE: changed_cb(int, int, int, int, char const*, void*) (editor.cxx:596)
==20970== by 0x4214A5: Fl_Text_Buffer::call_modify_callbacks(int, int, int, int, char const*) const (Fl_Text_Buffer.cxx:1311)
==20970== by 0x41F548: Fl_Text_Buffer::remove(int, int) (Fl_Text_Buffer.cxx:355)
==20970== by 0x4213A3: Fl_Text_Buffer::remove_selection_(Fl_Text_Selection*) (Fl_Text_Buffer.cxx:1272)
==20970== by 0x41FAD2: Fl_Text_Buffer::remove_selection() (Fl_Text_Buffer.cxx:532)
==20970== by 0x405675: Fl_Text_Buffer::loadfile(char const*, int) (Fl_Text_Buffer.H:310)
==20970== by 0x406513: load_file(char const*, int) (editor.cxx:486)
==20970== Address 0x53c6871 is 0 bytes after a block of size 1 alloc'd
Focusing on handle_vline(), I forced an abort() in the code valgrind identified
when it tried to access past the end of the buffer, then re-ran the editor demo in gdb
to get more useful output that includes the argument values, and let me probe around
with the live code:
#0 0x000000380d232625 in raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1 0x000000380d233e05 in abort () at abort.c:92
#2 0x00000000004266ce in Fl_Text_Display::handle_vline (this=0x68fe40, mode=3, lineStartPos=0, lineLen=14, leftChar=0, rightChar=0, Y=0, bottomClip=0, leftClip=0,
rightClip=0) at Fl_Text_Display.cxx:1938
#3 0x0000000000424ada in Fl_Text_Display::position_to_xy (this=0x68fe40, pos=14, X=0x7fffffffd650, Y=0x7fffffffd64c) at Fl_Text_Display.cxx:1025
#4 0x0000000000424f62 in Fl_Text_Display::display_insert (this=0x68fe40) at Fl_Text_Display.cxx:1172
#5 0x0000000000423bf5 in Fl_Text_Display::resize (this=0x68fe40, X=2, Y=32, W=656, H=366) at Fl_Text_Display.cxx:622
#6 0x00000000004250d5 in Fl_Text_Display::show_insert_position (this=0x68fe40) at Fl_Text_Display.cxx:1199
#7 0x0000000000406aff in changed_cb (nInserted=0, nDeleted=14, v=0x68f0b0) at editor.cxx:596
#8 0x00000000004214f6 in Fl_Text_Buffer::call_modify_callbacks (this=0x68e710, pos=0, nDeleted=14, nInserted=0, nRestyled=0, deletedText=0x752720 "asdf\nasdf\nasdf")
at Fl_Text_Buffer.cxx:1311
#9 0x000000000041f599 in Fl_Text_Buffer::remove (this=0x68e710, start=0, end=14) at Fl_Text_Buffer.cxx:355
#10 0x00000000004213f4 in Fl_Text_Buffer::remove_selection_ (this=0x68e710, sel=0x68e720) at Fl_Text_Buffer.cxx:1272
#11 0x000000000041fb23 in Fl_Text_Buffer::remove_selection (this=0x68e710) at Fl_Text_Buffer.cxx:532
#12 0x00000000004056c6 in Fl_Text_Buffer::loadfile (this=0x68e710, file=0x7527f0 "/tmp/foo.txt", buflen=131072) at ../FL/Fl_Text_Buffer.H:310
#13 0x0000000000406564 in load_file (newfile=0x7527f0 "/tmp/foo.txt", ipos=-1) at editor.cxx:486
#14 0x0000000000406bc6 in open_cb () at editor.cxx:613
#15 0x0000000000417d56 in Fl_Menu_Item::do_callback (this=0x68f8c0, o=0x68f7b0) at ../FL/Fl_Menu_Item.H:381
#16 0x00000000004185e4 in Fl_Menu_::picked (this=0x68f7b0, v=0x68f8c0) at Fl_Menu_.cxx:271
#17 0x00000000004190a8 in Fl_Menu_Bar::handle (this=0x68f7b0, event=1) at Fl_Menu_Bar.cxx:55
[..]
My take on this:
1) When changed_cb() is invoked (#7) the old buffer contents has already been remove()ed
2) When display_insert() is called, it starts working with the 'current' cursor position mCursorPos
which at this point in time has stale contents of the old cursor position before the buffer
was cleared.
3) This non-zero cursor position is now pointing beyond the end of the actual text buffer,
but is passed as an argument to Fl_Text_Display::position_to_xy() (#3: pos=14)
which does calculations that leads to considering data beyond the end of the buffer
in handle_hline(), reading random buffer data (NULLs and non-ascii binary gibberish)
There are many possible solutions; the core code could be strengthened, and the editor demo
could be modified.
Any of the following core mods could solve:
o The code that deletes text from the buffer should first adjust the cursor position
to make sure it points to a valid position before calling the modify callback
o Range check position_to_xy()'s first argument to see if it's off the end of the buffer()
o Other possible solutions..
Also: the application code (editor.cxx) may not be coded correctly:
o Should changed_cb() really be calling show_insert_position() during 'loading' only?
Perhaps the logic is inverted, and should instead be 'if (!loading) show_insert_position()'?
I'm not sure why the scroll position needs to be adjusted during any kind of changes..
shouldn't the editor handle this?
Not sure what the correct solution(s) are -- seeking input from devs. | |
| |
| #2 | greg.ercolano
23:11 May 25, 2015 |
| Minimal fix suggestion to range check position_to_xy()
attached as position_to_xy-range-check.patch | |
| |
| #3 | AlbrechtS
07:38 May 26, 2015 |
| Sorry, no time to read more thoroughly and follow you..
Only one thought WRT "Should changed_cb() really be calling show_insert_position() during 'loading' only?":
Maybe this is done to show the end of the loaded text or it is done to adjust the scrollbar so you can see that more text is added to the buffer, because the scrollbar's trough changes size and position. Other than that guess I don't have an idea. | |
| |
| #4 | AlbrechtS
08:43 Sep 01, 2016 |
| Coming back to this. Well, I tested and tried to debug the issue, but I came to the conclusion that a *good* fix would be difficult to achieve. The easiest fix we can do for now would be to commit the proposed patch position_to_xy-range-check.patch.
@Greg: great analysis of what happens!
I'd like to add: the trouble lies in the separation of the Fl_Text_Buffer and the Fl_Text_Editor object, which is generally good but makes things complicated. I believe that the editor.cxx code is legal (doesn't do anything wrong), but:
editor.cxx, line #583 calls Fl_Text_Buffer::loadfile() which removes the old buffer contents and loads new contents into the buffer. This eventually calls changed_cb() as Greg also found out. So far this all has only dealt with the Fl_Text_Buffer object which doesn't "know" anything of the Fl_Text_Editor it is associated with.
editor.cxx, line #695 (changed_cb) now calls Fl_Text_Display::show_insert_position() which calls resize() to do the dirty work. This doesn't look wrong.
The problem is that the Fl_Text_Editor object doesn't _seem_ to "know" that the contents of its Fl_Text_Buffer object changed and uses all its internal status variables (again, as Greg already found out). But why?
Fl_Text_Editor (aka Fl_Text_Display) _should_ know that the buffer contents changed. I verified that its own callback Fl_Text_Display::buffer_modified_cb() is called whenever the buffer is changed, so basically we can take account of buffer changes. This callback function is pretty long and does several things.
... testing ...
Well, things can be complicated and confusing. Although Fl_Text_Display's own callback Fl_Text_Display::buffer_modified_cb() *is* called, it is called *after* the user's callback (in this case editor.cxx:changed_cb()). Catch-22. Since Fl_Text_Display's callback has not yet been called and changed_cb() calls Fl_Text_Display's show_insert_position() the latter *has* stale data.
This needs more investigation. Maybe we must forbid to call Fl_Text_Display methods in buffer_modify_callbacks? In this case editor.cxx would be at fault. OTOH there's probably other code out there that does such things. I don't know what the best solution may be, but I ran out of time. Anyway, I wanted to write this down for later. | |
| |
| #5 | AlbrechtS
08:49 Sep 01, 2016 |
| Anyway, I'd like to get this fixed _somehow_ before we release 1.3.4, hence I raised priority from 2 (low) to 4 (high). Out of memory access is always bad and can lead to program crashes.
Greg, I think we should commit your patch but leave the STR open (maybe move to 1.4?). This does only fix the symptoms, but it's kinda defensive programming and can't do any harm for FLTK 1.3.4. We can then find a better solution for FLTK 1.4. What do you think? | |
| |
| #6 | greg.ercolano
08:55 Sep 01, 2016 |
| Uh.."yes, that sounds good".
Funny thing is I have completely forgotten this whole issue.
The more I read it, the more I go "wow, this guy really went down
the rabbit hole chasing this one." | |
| |
| #7 | AlbrechtS
09:09 Sep 01, 2016 |
| Yes, I did. More than I had intended. It took hours... | |
| |
| #8 | greg.ercolano
14:20 Oct 01, 2016 |
| OK, applied to 1.3 current as r12000. | |
| |
| #9 | greg.ercolano
14:24 Oct 01, 2016 |
| Fixed in Subversion repository.
Leaving this in the pending state, in case Albrecht wants to
address the other issues raised in some way. | |
| |
| #10 | AlbrechtS
15:17 Oct 01, 2016 |
| Thanks for fixing this. That was also my idea how to solve it for now.
Honestly, I have no concrete idea how we could fix the root cause. The kind of these Fl_Buffer callbacks and its interaction with the Fl_Text_Display and/or Fl_Text_Editor widget makes it difficult if not impossible to fix this. IMNSHO the Fl_Text_Display/Editor widget should get the callback first, before all other callbacks are called.
One option might be to have priorities in the Fl_Buffer callback queue so that the Fl_Text_Display widget could register its callback with higher priority so it is called first, or something like that. Maybe another "internal FLTK" callback queue, I don't know. Such a priority callback system would be too difficult to implement and test now and might break the ABI, hence I'm bumping this STR to 1.4-feature. I'm leaving priority at 4 (high) though, so we don't forget this STR before we go to release 1.4.0. I hope you don't mind. Feel free to move it back to 1.3-current, but with priority lower than 4 since it's no longer critical (i.e. not high priority) and should not block the release of 1.3.4.
FTR: Changed "Software Version" from 1.3-current to 1.4-feature. | |
| |
| #11 | AlbrechtS
04:15 Apr 21, 2021 |
| The root cause of this STR *may* be related to GitHub Issue #89 "Intermittent crash from Fl_Simple_Terminal dtor on Windows".
https://github.com/fltk/fltk/issues/89
https://github.com/fltk/fltk/issues/89#issuecomment-823978572
This STR and Issue #89 should be evaluated together... | |
| |
| #12 | AlbrechtS
05:32 Apr 21, 2021 |
| FWIW: the mentioned fix in svn r12000 has also been committed in svn r12001 to the so-called "porting" branch at that time which is now the 'master' branch (1.4.0 development). Git commit ID's are (for later reference):
svn r12000 - branch-1.3 - 7651217fa3f013ae8afe0d81dd0599c26c3bdfb5
svn r12001 - master - a82eed18e5a9d0984d607127bddc95f5f8690803
The fix in branch-1.3 is included in FLTK 1.3.4. | |
[ Return to Bugs & Features | Post Text | Post File ]
|
| |
