FLTK logo

STR #2730

FLTK matrix user chat room
(using Element browser app)   FLTK gitter user chat room   GitHub FLTK Project   FLTK News RSS Feed  
  FLTK Apps      FLTK Library      Forums      Links     Login 
 Home  |  Articles & FAQs  |  Bugs & Features  |  Documentation  |  Download  |  Screenshots  ]
 

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:

Receive EMails Don't Receive EMails

Trouble Report Files:


Name/Time/Date Filename/Size  
 
#1 AlbrechtS
16:47 Oct 28, 2014
Fl_Text_Display.patch
1k
 
 
#2 AlbrechtS
16:47 Oct 28, 2014
test_editor.diff
1k
 
     

Trouble Report Comments:


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

 
 

Comments are owned by the poster. All other content is copyright 1998-2024 by Bill Spitzak and others. This project is hosted by The FLTK Team. Please report site problems to 'erco@seriss.com'.