[ Return to Bugs & Features | Post Text | Post File | Prev | Next ]

STR #3231

Trouble Report Files:

	Name/Time/Date	Filename/Size
	#1  greg.ercolano 23:11 May 25, 2015	position_to_xy-range-check.patch 1k

Trouble Report Comments:

	Name/Time/Date	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 ]