bugGforth - Bugs: bug #46988, Fix use of uninitalized memory

 
 

bug #46988: Fix use of uninitalized memory

Submitted by:  Jasper Lievisse Adriaanse <jasperla>
Submitted on:  Wed 27 Jan 2016 08:10:28 AM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: NoneStatus: Fixed
Privacy: PublicAssigned to: Anton Ertl <anton>
Open/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Thu 28 Jan 2016 06:10:10 PM UTC, comment #10:

ok, I should learn that the summary is the title of the bug:-)

Anton Ertl <anton>
Project AdministratorIn charge of this item.
Wed 27 Jan 2016 06:21:45 PM UTC, comment #9:

Thanks, the patch from Anton resolves the issue here with MALLOC_OPTIONS=S (implies J).

Jasper Lievisse Adriaanse <jasperla>
Wed 27 Jan 2016 06:18:19 PM UTC, comment #8:

Line 418 for gforth-0.7.3, line 480 for git head, after the closing bracket of this loop:

for (; routines[i]!=0; i++) {
...
}

Bernd Paysan <paysan>
Project Administrator
Wed 27 Jan 2016 06:17:17 PM UTC, comment #7:

The attached patch should do it.

(file #36181)

Anton Ertl <anton>
Project AdministratorIn charge of this item.
Wed 27 Jan 2016 06:06:41 PM UTC, comment #6:

Could you please give me some context on the line? As http://git.savannah.gnu.org/cgit/gforth.git/tree/engine/engine.c#n418 seems the wrong place.

Jasper Lievisse Adriaanse <jasperla>
Wed 27 Jan 2016 05:55:32 PM UTC, comment #5:

Looks like the culprit is the relocate loop while symbol[i]!=0. That last zero doesn't get copied. If I copy that last 0, valgrind reports nothing for gforth-ditc.

So I suggest to keep the malloc and correctly set the terminating 0 for the allocated arrays. That way we can check for unintended use of the uninitialized part of the array.

Add the following before line 418 in engine/engine.c:

xts[i] = symbols[i] = 0;

and try if the bug persists (with the mallocs).

Bernd Paysan <paysan>
Project Administrator
Wed 27 Jan 2016 05:38:28 PM UTC, comment #4:

I can reproduce the crash 100% of the time:

GNU gdb 6.3
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB. Type "show warranty" for details.
This GDB was configured as "amd64-unknown-openbsd5.9"...
Core was generated by `gforth-ditc'.
Program terminated with signal 11, Segmentation fault.
Reading symbols from /usr/lib/libpthread.so.20.1...done.
Loaded symbols for /usr/lib/libpthread.so.20.1
Loaded symbols for /usr/obj/ports/gforth-0.7.3/gforth-0.7.3/gforth-ditc
Reading symbols from /usr/local/lib/libavcall.so.1.0...done.
Loaded symbols for /usr/local/lib/libavcall.so.1.0
Reading symbols from /usr/local/lib/libffi.so.1.2...done.
Loaded symbols for /usr/local/lib/libffi.so.1.2
Reading symbols from /usr/local/lib/libltdl.so.5.0...done.
Loaded symbols for /usr/local/lib/libltdl.so.5.0
Reading symbols from /usr/lib/libm.so.9.0...done.
Loaded symbols for /usr/lib/libm.so.9.0
Symbols already loaded for /usr/lib/libpthread.so.20.1
Reading symbols from /usr/lib/libc.so.84.2...done.
Loaded symbols for /usr/lib/libc.so.84.2
Reading symbols from /usr/libexec/ld.so...done.
Loaded symbols for /usr/libexec/ld.so
#0 0x00001e33f031ec7b in check_prims (symbols1=0x1e35fbc01000) at ./main.c:954
954 for (i=0; symbols1[i]!=0; i++)
(gdb) bt
#0 0x00001e33f031ec7b in check_prims (symbols1=0x1e35fbc01000) at ./main.c:954
#1 0x00001e33f031ef25 in gforth_loader (imagefile=0x1e3619f01100, filename=0x1e33f0423970 "gforth.fi")
at ./main.c:1860
#2 0x00001e33f031fdd1 in main (argc=15, argv=0x7f7ffffcf538, env=0x7f7ffffcf5b8) at ./main.c:2323
(gdb) bt full
#0 0x00001e33f031ec7b in check_prims (symbols1=0x1e35fbc01000) at ./main.c:954
i = 1536
#1 0x00001e33f031ef25 in gforth_loader (imagefile=0x1e3619f01100, filename=0x1e33f0423970 "gforth.fi")
at ./main.c:1860
header = {base = 0x1e3619f01100 "", checksum = 16837413913267837873, image_size = 140187732341696,
dict_size = 33208421971100, data_stack_size = 7165064483209180463, fp_stack_size = 7435269714738113633,
return_stack_size = 3328212250303557478, locals_stack_size = 8245922036098215479,
boot_entry = 0x74720069662e6874, throw_entry = 0x1e0069662e68, unused1 = 33217707870848,
xt_base = 0x1e33f031f40a, data_stack_base = 0x7f7ffffcf330 "/usr/local/lib/gforth/0.7.3/gforth.fi",
fp_stack_base = 0x7f7ffffcf370 "0\177\177", return_stack_base = 0x1e33f0423970 "gforth.fi",
locals_stack_base = 0x1b8a3e9a00 <Address 0x1b8a3e9a00 out of bounds>}
image = 0x7f7ffffcf803 "/usr/local/share/gforth/0.7.3:."
imp = 0x1 <Address 0x1 out of bounds>
magic = "\002\177\177\000"
magic7 = 0 '\0'
preamblesize = 0
data_offset = 0
check_sum = 140187732341776
ausize = 0
charsize = 0
cellsize = 3
sizebyte = 7
#2 0x00001e33f031fdd1 in main (argc=15, argv=0x7f7ffffcf538, env=0x7f7ffffcf5b8) at ./main.c:2323
path = 0x7f7ffffcf7a1 ".:/usr/local/lib/gforth/site-forth:/usr/local/share/gforth/site-forth:/usr/local/lib/gforth/0.7.3:/usr/local/share/gforth/0.7.3:."
imagename = 0x1e33f0423970 "gforth.fi"
image_file = (FILE *) 0x1e3619f01100
image = 0x7f7ffffcf5b8 "\177\177"
retvalue = 0
(gdb)

Jasper Lievisse Adriaanse <jasperla>
Wed 27 Jan 2016 05:34:58 PM UTC, comment #3:

Valgrind reports some usage of uninitialized memory with gforth-ditc, the other engines don't have such reports.

> valgrind --track-origins=yes ./gforth-ditc

==31224== Memcheck, a memory error detector
==31224== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==31224== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==31224== Command: ./gforth-ditc
==31224==
==31224== Conditional jump or move depends on uninitialised value(s)
==31224== at 0x40AC1C: gforth_loader (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== by 0x40BCF9: gforth_start (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== by 0x40BDA5: gforth_main (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== by 0x404BE8: main (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== Uninitialised value was created by a heap allocation
==31224== at 0x4C2A00F: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==31224== by 0x404DA2: gforth_engine (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== by 0x40ABE2: gforth_loader (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== by 0x40BCF9: gforth_start (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== by 0x40BDA5: gforth_main (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== by 0x404BE8: main (in /home/bernd/gforths/gforth/gforth-ditc)
==31224==
==31224== Conditional jump or move depends on uninitialised value(s)
==31224== at 0x40A544: gforth_relocate (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== by 0x40B1B2: gforth_loader (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== by 0x40BCF9: gforth_start (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== by 0x40BDA5: gforth_main (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== by 0x404BE8: main (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== Uninitialised value was created by a heap allocation
==31224== at 0x4C2A00F: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==31224== by 0x404DA2: gforth_engine (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== by 0x40ABE2: gforth_loader (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== by 0x40BCF9: gforth_start (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== by 0x40BDA5: gforth_main (in /home/bernd/gforths/gforth/gforth-ditc)
==31224== by 0x404BE8: main (in /home/bernd/gforths/gforth/gforth-ditc)
==31224==

This report goes away with the calloc() fix. Setting the arrays to junk doesn't cause a crash even with gforth-ditc; so I read it as "might crash" rather than "does crash".

Bernd Paysan <paysan>
Project Administrator
Wed 27 Jan 2016 05:07:56 PM UTC, comment #2:

Yes, the uninitialized memory should not be used. But if the reporter observed a crash, it apparently is used, and that is a bug that is only superficially fixed by the suggested patch.

So I am reopening it and will look into it.

Anton Ertl <anton>
Project AdministratorIn charge of this item.
Wed 27 Jan 2016 04:43:21 PM UTC, comment #1:

Hm, all used xts and symbols are correctly initialized. There are these offsets which are used for helping relocation, this means that some parts of the array is not used, and thus also never initialized.

Using calloc will remove any false positive detection, though.

Bernd Paysan <paysan>
Project Administrator
Wed 27 Jan 2016 08:10:28 AM UTC, original submission:

OpenBSD's malloc(3) (see [1]) implementation allows for adding junk to the allocated area (see options 'j' and 'J'). Through this it was discovered that gforth_engine() was using uninitialized memory and would crash. The attached patch resolves the issue.

1: http://www.openbsd.org/cgi-bin/man.cgi?query=malloc.conf

Jasper Lievisse Adriaanse <jasperla>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #36181:  patch added by anton (391B - application/octet-stream - fix)
file #36171:  patch-engine_engine_c added by jasperla (853B - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by anton (Posted a comment)
  • -unavailable- added by paysan (Posted a comment)
  • -unavailable- added by jasperla (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 14 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 28 Jan 2016 06:10:10 PM UTCantonSummaryFixed in git head=>Fix use of uninitalized memory
    Thu 28 Jan 2016 06:03:43 PM UTCantonStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
      SummaryFix use of uninitalized memory =>Fixed in git head
    Wed 27 Jan 2016 06:18:19 PM UTCpaysanStatusConfirmed=>In Progress
    Wed 27 Jan 2016 06:17:17 PM UTCantonAttached File-=>Added patch, #36181
      StatusIn Progress=>Confirmed
    Wed 27 Jan 2016 05:07:56 PM UTCantonStatusFixed=>In Progress
      Assigned topaysan=>anton
      Open/ClosedClosed=>Open
    Wed 27 Jan 2016 04:45:54 PM UTCpaysanStatusNone=>Fixed
      Assigned toNone=>paysan
      Open/ClosedOpen=>Closed
    Wed 27 Jan 2016 08:10:28 AM UTCjasperlaAttached File-=>Added patch-engine_engine_c, #36171

    Back to the top


    Powered by Savane 3.1-cleanup1