Re: Crash report - segfault reproduceable

Nick Warne wrote:
 > On Tuesday 31 January 2006 00:05, Mike Frysinger wrote:
 >> i found that i could also reproduce this in Windows by logging in
 >> with putty, exporting TERM=putty, and then resizing the terminal
 >> -mike
 >
 > Well I can get this to segfault easy now, even on opening the test
 > file if I do not resize the window when it does segfault - if you need
 > to run some debug code to trace, I am willing to help.
 >
 > I have looked at the code, but I think you need to be a nano veteran
 > to follow it - it's a bit awkward to follow for a virgin...

No problem.  Would you try getting the latest CVS, applying the
following patch, running nano WITHOUT the -O/--morespace option (as the
debugging output is on the line that that option makes part of the edit
window) so that it crashes, and telling me what results are shown during
the entire session?  Thanks in advance.

diff -ur nano/src/winio.c nano-fixed/src/winio.c
--- nano/src/winio.c	2006-01-30 16:23:51.000000000 -0500
+++ nano-fixed/src/winio.c	2006-02-02 17:55:41.000000000 -0500
 <at>  <at>  -1922,6 +1922,10  <at>  <at> 

     free(buf_mb);

+    mvwprintw(topwin, 1, 0, "line %ld: allocated %lu bytes, index == %lu, converted length == %lu\n",
+	(openfile == NULL) ? -1 : (long)openfile->current->lineno, (unsigned long)(alloc_len + 1),

Re: Crash report - segfault reproduceable

On Thursday 02 February 2006 18:02, David Lawrence Ramsey wrote:
> No problem.  Would you try getting the latest CVS, applying the
> following patch, running nano WITHOUT the -O/--morespace option (as the
> debugging output is on the line that that option makes part of the edit
> window) so that it crashes, and telling me what results are shown during
> the entire session?  Thanks in advance.

well if i use a terminal size of 81x24 it crashes at startup:
line 1: allocated 83 bytes, index == 88, converted length == 89

also, it'd be easier to capture the output if that statement wrote to 
stderr ...
-mike

Re: Crash report - segfault reproduceable

Mike Frysinger wrote:
 > On Thursday 02 February 2006 18:02, David Lawrence Ramsey wrote:
 >> No problem.  Would you try getting the latest CVS, applying the
 >> following patch, running nano WITHOUT the -O/--morespace option (as
 >> the debugging output is on the line that that option makes part of
 >> the edit window) so that it crashes, and telling me what results are
 >> shown during the entire session?  Thanks in advance.
 >
 > well if i use a terminal size of 81x24 it crashes at startup:
 > line 1: allocated 83 bytes, index == 88, converted length == 89

I can reproduce this now.  I've been using UTF-8 mode exclusively for
awhile now, and the size of MB_CUR_MAX apparently provides enough of a
buffer that the buffer overflow in display_string() that you're seeing
doesn't happen.  Current CVS allocates COLS characters per line, and the
extra tab on the end overflows it, so adding room for tabsize
characters, as in the attached patch, should fix it.  Please let me know
if it does (and please apply it on top of the other patch so that I can
be sure).

Note that this doesn't fix the problem with the "coretest" file, so it's
most likely a separate problem.

 > also, it'd be easier to capture the output if that statement wrote to
 > stderr ...

True, but it'd be harder to see what was onscreen, which is why I did it
that way.  If the attached patch doesn't work, I'll make a new version
of the other patch.

Re: Crash report - segfault reproduceable

On Thursday 02 February 2006 19:50, David Lawrence Ramsey wrote:
> I can reproduce this now.  I've been using UTF-8 mode exclusively for
> awhile now, and the size of MB_CUR_MAX apparently provides enough of a
> buffer that the buffer overflow in display_string() that you're seeing
> doesn't happen.  Current CVS allocates COLS characters per line, and the
> extra tab on the end overflows it, so adding room for tabsize
> characters, as in the attached patch, should fix it.  Please let me know
> if it does (and please apply it on top of the other patch so that I can
> be sure).

ok, the coretest file crashed less often, but here's the output after moving 
to a 90x24 terminal:
line 1: allocated 100 bytes, index == 104, converted length == 105
-mike

Re: Crash report - segfault reproduceable

Mike Frysinger wrote:

<snip>

 >ok, the coretest file crashed less often, but here's the output after
 >moving to a 90x24 terminal:
 >line 1: allocated 100 bytes, index == 104, converted length == 105

I think I've finally fixed it, since neither of your test cases seem to
crash current CVS anymore.  Could you get current CVS again (note that
the patch won't apply cleanly to it anymore, but that's because I
reworked the memory allocation; sorry for any inconvenience; you can use
--enable-debug instead, since I added an assert that index will never be
out of range of alloc_len) and try it?  Thanks in advance.

Re: Crash report - segfault reproduceable

On Thursday 02 February 2006 23:21, David Lawrence Ramsey wrote:
> I think I've finally fixed it, since neither of your test cases seem to
> crash current CVS anymore.

indeed ... constantly resizing and pounding the arrow keys yields no crash

kudos !
-mike

Re: Crash report - segfault reproduceable

Mike Frysinger wrote:

<snip>

 > indeed ... constantly resizing and pounding the arrow keys yields no
 > crash
 >
 > kudos !

Thank you.  I'm attaching patches to add the same fix to 1.3.7 through
1.3.10, since this is a longstanding problem.

diff -ur nano-1.3.7/src/winio.c nano-1.3.7-fixed/src/winio.c
--- nano-1.3.7/src/winio.c	2005-04-10 23:51:22.000000000 -0400
+++ nano-1.3.7-fixed/src/winio.c	2006-02-02 23:47:01.000000000 -0500
 <at>  <at>  -2253,11 +2253,22  <at>  <at> 

     assert(column <= start_col);

-    /* Allocate enough space for the entire line.  It should contain
-     * (len + 2) multibyte characters at most. */
-    alloc_len = mb_cur_max() * (len + 2);
+    /* Make sure there's enough room for the initial character, whether
+     * it's a multibyte control character, a non-control multibyte
+     * character, a tab character, or a null terminator.  Rationale:
+     *
+     * multibyte control character followed by a null terminator:
+     *     1 byte ('^') + mb_cur_max() bytes + 1 byte ('\0')

Re: About the nano rewrite

David Benbennick wrote:

<snip>

 > There's a new version of the code at
 > http://www.cam.cornell.edu/~dbenbenn/new-nano.tar.gz.  The new version
 > has rebindable keys.

Oh good.

 > Undo.cc wasn't supposed to be there; it's gone now.  The undo code is
 > in TextBuffer.cc:do_undo().  I suspect that what you did is type some
 > characters, then hit ^Z?  In the old version, undoing wouldn't become
 > active until you did something destructive (like delete or cut).  I've
 > changed it a bit, though it still doesn't do exactly the "right
 > thing".

Okay.  Thanks for the explanation.  I'll try it out as soon as I can.

<snip>

 > I'll wrap the code that needs Boost in precompiler flags.  It's only
 > used for regex searching, anyway.  I'll send another email when that's
 > done.

By the way, is the dependency on Boost a temporary holdover from the
original kubux code, or is it going to be an integral part of "onan"?

<snip>

Re: Crash report - segfault reproduceable

On Friday 03 February 2006 04:49, David Lawrence Ramsey wrote:
> Mike Frysinger wrote:
>
> <snip>
>
>  > indeed ... constantly resizing and pounding the arrow keys yields no
>  > crash
>  >
>  > kudos !
>
> Thank you.  I'm attaching patches to add the same fix to 1.3.7 through
> 1.3.10, since this is a longstanding problem.

Good job!  I applied cvs here at home and I cannot get any segfault repeating 
my otherwise 'reproduceable' crashes!  I will repeat at work today, but it 
looks good.

Thank you!

Nick
--

-- 
"Person who say it cannot be done should not interrupt person doing it."
-Chinese Proverb

Re: About the nano rewrite

On 2/3/06, David Lawrence Ramsey <pooka109 <at> cox.net> wrote:
> By the way, is the dependency on Boost a temporary holdover from the
> original kubux code, or is it going to be an integral part of "onan"?

Boost is pretty much required for good regex search.

We could have a fallback to the POSIX regex functions, I suppose.  The
problem is that they search in char* strings.  In Nano, that's no
problem, since the file is stored as a list of char*s.  In the new
version, the file is stored as a single Text object.  So to search
with the POSIX functions, you'd have to convert each line to a
temporary char*.  The code probably wouldn't be too complicated, but
it would be inefficient.  (If you like, I could implement it and run
some benchmarks to see exactly how inefficient.)

(Another alternative would be to include a version of the Boost regex
code with Nano.  The magic configure script could tell the Makefile to
use the included code if Boost isn't already installed on the system.)

Something I'd like to move away from with the new code is the
proliferation of precompiler variables.  I've intentionally left out
flags like DISABLE_JUSTIFY, etc.  The problem is that they make the
code very hard to maintain.  When's the last time you actually tested
Nano with DISABLE_JUSTIFY, DISABLE_MOUSE, and DISABLE_TABCOMP set, and
all the other features enabled?  All the different flags mean there
are hundreds of different "versions" of Nano to support.

I suppose it still makes sense to have a NANO_SMALL flag (there are
still people out there who use floppy boot disks, I guess), but I
don't think anyone really needs the ability to turn off help support