| [ Return to Bugs & Features | Roadmap 1.3 | SVN ⇄ GIT ]
STR #2730
Application: | FLTK Library |
Status: | 1 - Closed w/Resolution |
Priority: | 4 - High, e.g. key functionality not working |
Scope: | 3 - Applies to all machines and operating systems |
Subsystem: | Core Library |
Summary: | valgrind, out of bounds access, Fl_Text_Display wrapping |
Version: | 1.3-current |
Created By: | corvid |
Assigned To: | AlbrechtS |
Fix Version: | 1.3.3 (SVN: v10415) |
Update Notification: | |
Trouble Report Files:
Trouble Report Comments:
|
#1 | corvid 21:06 Oct 10, 2011 |
| The fix for the access at offset -1 (STR #2691) causes an out of bounds access at the other end, e.g.:
==22171== Invalid read of size 1 ==22171== at 0x80DA58F: Fl_Text_Display::measure_proportional_character(char const*, int, int) const (Fl_Text_Display.cxx:3220) ==22171== by 0x80DA42B: Fl_Text_Display::wrapped_line_counter(Fl_Text_Buffer*, int, int, int, bool, int, int*, int*, int*, int*, bool) const (Fl_Text_Display.cxx:3164) ==22171== by 0x80D69DD: Fl_Text_Display::count_lines(int, int, bool) const (Fl_Text_Display.cxx:1162) ==22171== by 0x80D4C5B: Fl_Text_Display::resize(int, int, int, int) (Fl_Text_Display.cxx:320) ==22171== by 0x80D72BD: Fl_Text_Display::buffer_modified_cb(int, int, int, int, char const*, void*) (Fl_Text_Display.cxx:1485) ==22171== by 0x80D3483: Fl_Text_Buffer::call_modify_callbacks(int, int, int, int, char const*) const (Fl_Text_Buffer.cxx:1310) ==22171== by 0x80D1565: Fl_Text_Buffer::text(char const*) (Fl_Text_Buffer.cxx:188) ==22171== Address 0x4867457 is 0 bytes after a block of size 1,583 alloc'd ==22171== at 0x402562C: malloc (vg_replace_malloc.c:236) ==22171== by 0x80D14F1: Fl_Text_Buffer::text(char const*) (Fl_Text_Buffer.cxx:178)
p is buf->length()-1, and so is b initially. "if (b<lineStart) b = lineStart;" increases it to buf->length(). "const char *s = buf->address(b);" gets a value just beyond the end of the buffer. | |
|
#2 | corvid 21:11 Oct 10, 2011 |
| BTW, the line numbers in the valgrind report are off by a few because I have a couple of printf()s in my Fl_Text_Display.cxx, but all of its complaints are when *s is used. | |
|
#3 | matt 13:54 Dec 10, 2011 |
| I wish I knew how to use Valgrind on OS X. Back in the Linux days it was by far the best debugging method, and I used it on huge project to find lots of those nasty unrepeatable bugs.
This should be easy to fix. I'll take a look at it after updating the STRs. | |
|
#4 | fabien 18:37 Apr 20, 2012 |
| http://valgrind.org/downloads/current.html#current (now works on os x 10.6 & 10.7)
So what's the status of that problem now, is it still happening ? | |
|
#5 | corvid 23:00 Oct 02, 2012 |
| Yes, we still get plenty of these errors, as you can see at http://starurchin.org/dillo/valgrind.html | |
|
#6 | greg.ercolano 07:07 Oct 03, 2012 |
| Is there any replication info that can be supplied? Optimally, a compilable foo.cxx that demonstrates the problem and some instructions on how to cause the issue (eg. compile+run foo.cxx, and then resize the window slowly to cause the buffer overrun in valgrind) | |
|
#7 | ianmacarthur 02:00 Oct 17, 2012 |
| Corvid indicates this arose after fixing #2691... Matt, I think that was applied by you; do you know what is going on with this one?
The text editor code is practically opaque to me; I have no idea what is going on! | |
|
#8 | ianmacarthur 15:32 Mar 11, 2014 |
| Does anyone know if this still happens? Or was it fixed? | |
|
#9 | greg.ercolano 08:12 May 22, 2014 |
| Assigning to me.
Looks like Matt has fallen off the planet earth, so I'll see if I can figure it out.
Seems the fix for STR#2691 was the change in svn r9065:
_____________________________________________________________________ Author: matt Date: 2011-09-27 10:10:04 -0700 (Tue, 27 Sep 2011) New Revision: 9065 Log: STR #2691: fixed possible invalid text pointer in Text Display Modified: branches/branch-1.3/src/Fl_Text_Display.cxx Modified: branches/branch-1.3/src/Fl_Text_Display.cxx =================================================================== --- branches/branch-1.3/src/Fl_Text_Display.cxx 2011-09-27 17:00:35 UTC (rev 9064) +++ branches/branch-1.3/src/Fl_Text_Display.cxx 2011-09-27 17:10:04 UTC (rev 9065) @@ -3133,6 +3133,7 @@ break; } } + if (b<lineStart) b = lineStart; if (!foundBreak) { /* no whitespace, just break at margin */ newLineStart = max(p, buf->next_char(lineStart)); const char *s = buf->address(b); _______________________________________________________________ | |
|
#10 | greg.ercolano 15:49 May 22, 2014 |
| Are there any special build or valgrind flags you're using here? I can't replicate.
OP, if you would, please provide some replication details: 1) Platform/compiler 2) Simple app to demo issue (e.g. mod version of test/editor.cxx) 3) Special valgrind/compiler flags 4) User interaction needed to replicate error
What I've tried: 1) On both SciLinux6.3 and OSX 10.6 with default g++ compiler
2) Modified editor.cxx test program to enable WRAP_AT_BOUNDS: e->editor->wrap_mode(Fl_Text_Display::WRAP_AT_BOUNDS, 0);
3) Tried various invocations of valgrind: valgrind test/editor valgrind -v test/editor, valgrind --tool=exp-sgcheck test/editor (exp-sgcheck was available on linux only)
4) Tried typing long lines on the first line past the edge of the screen, including lines with one loooooong word (no spaces). Repeated these tests on other lines besides the first.
Could not get any errors out of valgrind with svn current. | |
|
#11 | corvid 09:05 May 25, 2014 |
| Sorry, had my old email addr at fltk.org so I wasn't notified of updates...
I can at least see that current dillo and current fltk still show the problem. I'll see what I can do to coerce test/editor into replicating the necessary conditions. | |
|
#12 | corvid 11:20 May 25, 2014 |
| Okay, I can get valgrind errors with:
$ svn diff Index: editor.cxx =================================================================== --- editor.cxx (revision 10173) +++ editor.cxx (working copy) @@ -796,15 +796,15 @@ w->begin(); Fl_Menu_Bar* m = new Fl_Menu_Bar(0, 0, 660, 30); m->copy(menuitems, w); - w->editor = new Fl_Text_Editor(0, 30, 660, 370); + w->editor = new Fl_Text_Editor(0, 30, 1, 1); w->editor->textfont(FL_COURIER); w->editor->textsize(TS); - //w->editor->wrap_mode(Fl_Text_Editor::WRAP_AT_BOUNDS, 250); + w->editor->wrap_mode(Fl_Text_Editor::WRAP_AT_BOUNDS, 0); w->editor->buffer(textbuf); w->editor->highlight_data(stylebuf, styletable, sizeof(styletable) / sizeof(styletable[0]), 'A', style_unfinished_cb, 0); - textbuf->text(); + textbuf->text("adsaf\n"); style_init(); w->end(); w->resizable(w->editor); | |
|
#13 | ianmacarthur 16:23 Sep 04, 2014 |
| Do we know; was this ever fixed? | |
|
#14 | corvid 17:01 Sep 04, 2014 |
| Still get valgrind errors. | |
|
#15 | ianmacarthur 02:56 Sep 05, 2014 |
| Does anyone have any insights here? I'm lost... | |
|
#16 | greg.ercolano 13:24 Oct 27, 2014 |
| It seems the OP was able to replicate with small mods to the FLTK test/editor, so this should be checked and if possible resolved.
Will give it a quick check today, and if I can't get anywhere, might have to assign this from myself, as I don't have as much time as I had when I first assigned this STR to me.
Not sure if this should be a show stopper for 1.3.3 release; dev comments? | |
|
#17 | greg.ercolano 18:17 Oct 27, 2014 |
| OK, I can replicate now with those mods shown in comment #12 using:
valgrind ./editor
I'm getting the "Invalid read" errors now for example this line in Fl_Text_Display::measure_proportional_character():
if (*s=='\t') {
..and some others (fl_utf8towc() and fl_utf8towc().
Will try to track it down.. | |
|
#18 | greg.ercolano 19:09 Oct 27, 2014 |
| Ya, I'm coming to similar conclusions as stated by the OP in comment #1; +1 byte off the end of the 1030 byte block. | |
|
#19 | AlbrechtS 16:47 Oct 28, 2014 |
| Please try the attached patch Fl_Text_Display.patch.
I don't get valgrind errors with this patch and the modifications in test/editor.cxx, as described in comment #12. For test consistency I'll upload the modifications I tested with as another patch file: see test_editor.diff.
Note that both are git diff's, and you must use `patch -p1` or similar to apply.
My test scenario:
(1) apply test_editor.diff, build (2) run `valgrind test/editor` --> see errors, as described in #1 [1] (3) apply Fl_Text_Editor.patch, build again (lib + test/editor) (4) run `valgrind test/editor` --> no errors
[1] Note: it's enough to start test/editor, the errors should show up immediately.
Please verify that this patch fixes the issue.
Please do also test that word wrapping still works as expected.
Status: this patch is a test, but it may not be the final fix. Maybe there are side effects on word wrapping - however I couldn't find any negative effects. Needs more testing, so please help. I'd like to include this patch or a similar one in FLTK 1.3.3 if it works as it should.
Any help, tests, comments etc. very much appreciated ! | |
|
#20 | manolo 02:21 Oct 29, 2014 |
| Verified the patch under Mac OS X: - the patch is indeed necessary because, without it, s points beyond the end of an allocated buffer, and *s is accessed - the patch is indeed efficient because, with it, *s is no longer accessed - enlarging the window of the modified editor program, it's possible to exercize wrapping, and to see it runs correctly.
Thus I believe the patch is quite useful and safe to apply now. | |
|
#21 | AlbrechtS 04:54 Oct 30, 2014 |
| Fixed in Subversion repository.
FTR: assigned this STR to me (from Greg).
I modified the proposed patch slightly, see 'svn diff -c 10415'.
Only b can trigger the out-of-bounds access, and the local variable s needs only be defined inside the relevant case. Test statement removed.
The test case doesn't show errors with this patch, as Manolo also confirmed (with the previous one).
Corvid, can you confirm that svn r10415 fixes the issue for you?
TIA | |
|
#22 | corvid 11:09 Oct 30, 2014 |
| I can see that the modified test/editor doesn't cause valgrind to complain anymore. As for fltk in dillo, I'm not aware of a reliable way to trigger the problem, so I can't be as definitive about this as I'd prefer... | |
|
#23 | AlbrechtS 04:28 Oct 31, 2014 |
| Thanks for confirmation so far. I'm closing this STR now.
If it turns out that there are still problems with dillo, please feel free to file another STR. | |
[ Return to Bugs & Features ]
|
| |