a repeatable 'more' crash and question about wide char and 'get_line' safety

* a repeatable 'more' crash and question about wide char and 'get_line' safety
@ 2013-07-16 23:20 Dr. David Alan Gilbert
  2013-07-19 22:35 ` [PATCH] get_line fixes for wide characters and overflows Dr. David Alan Gilbert
  0 siblings, 1 reply; 7+ messages in thread
From: Dr. David Alan Gilbert @ 2013-07-16 23:20 UTC (permalink / raw)
  To: util-linux; +Cc: rleigh, kzak

Hi,
  I've got a repeatable (for me!) crash in 'more' in git
( 5d3da4a24d0f952eb3709ba62c74aaa04729e08f ) that's also in at least 2.23.1 -
and I don't know how much older.

I believe the problem relates to the way the Line buffer is managed
in get_line; but more of the diagnosis below.

The problem:
------------

*** Error in `/usr/bin/more': free(): invalid pointer: 0x000000000060f850 ***

This was generated on OpenSuse 'factory' with their 2.23.1-4.3.x86_64
package, but then verified it on the current util-linux git.
Here is the bug I filed in their system:

https://bugzilla.novell.com/show_bug.cgi?id=829720

With the following test file:
https://bugzilla.novell.com/attachment.cgi?id=548234

(It's a fragment of /var/lib/rpm/Packages that I was
stupid enough to more).

The seg triggers in an 86x25 terminal for me just with

more tstfile2

It needs a UTF-8 LANG, I'm running with en_GB.UTF-8 but en_US.UTF-8 also
triggers it.

My original test triggered an invalid free check, but with the help
of valgrind I narrowed it down to:

==22488== Invalid write of size 1
==22488==    at 0x4037A2: get_line (more.c:1043)
        if (colflg && eatnl && Wrap) {
                *p++ = '\n';    /* simulate normal wrap */
        }
==22488==    by 0x4066C3: screen (more.c:660)
==22488==    by 0x4025E4: main (more.c:503)
==22488==  Address 0x542c318 is 0 bytes after a block of size 344 alloc'd
==22488==    at 0x4C297AB: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-lin

Answers/questions
-----------------

I've put a load of debug in and have some answers/questions:

   1) The loop in get_line() is:
        while (p < &Line[LineLen - 1]) {

     if I understand correctly that would seem to imply that if the loop
     always wrote no more than 1 character per iteration there should be 1
     space left in Line when you hit the bottom; of the loop, one for the
     \n in the code above that writes the '\n' sometimes....  but then
     it always writes a \0 on the end as well - so whenever we tack on
     a \n I think we overflow with the \0 if we're right at the end.

   2) unfortunately there are cases where the loop writes more than 1
   char per iteration; looking at:

http://git.kernel.org/cgit/utils/util-linux/util-linux.git/tree/text-utils/more.c#n874

     we see that the wide character code can take it's (size_t)-1 case,
     write a character and then it GOTO's (*) process_mbc: and hopefully
     lands in the default case and writes some characters in it's else
     clause; (in this test case 1) - but that loop worries me that it's
     possible to write more than one.

So hmm, what's the fix? 

    a) I think that loop limit needs to be at least LineLen - 2 just to
    cope with the \n and \0 case;  and there is also at least one inner
    loop (for tabs) that also has that loop limit coded in it that needs
    to match

    b) If we knew what the worst case was for the number of characters
    the WIDECHAR stuff could write then we could make the loop termination
    condition would then leave enoguh space for an entire widechar.

    c) Do we also need to change the allocation code in prepare_line_buffer?

It has:
              size_t nsz = Mcol * 4;

     Now that magic 4 isn't commented, but I'm currently guessing as
     it being the answer of (b) above - ie the size of a widechar?
     If so then I think it needs at least a +2 for the \n and \0

Dave (who doesn't know multi byte/wide stuff)

* Please hand Dijkstra a muffin
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\ gro.gilbert @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

^ permalink raw reply	[flat|nested] 7+ messages in thread