Re: [hatari-devel] Buffer overflow in Paths_Init()

[Thomas Huth <th.huth@xxxxxxxxx>]

Am Fri, 18 Jan 2019 01:13:08 +0200
schrieb Eero Tamminen <oak@xxxxxxxxxxxxxx>:

> Hi,
> 
> On 1/17/19 9:17 PM, Christian Zietz wrote:
> > for quite some time I have been puzzled why Hatari (snapshot builds
> > for Windows from antarctica.no) sometimes worked and sometimes
> > crashed with an HEAP_CORRUPTION error directly after start. Today,
> > I finally figured out that it works if I start it from the Windows
> > command line with an absolute path to the executable, but crashes
> > if I start it with just a relative path to the executable.
> > 
> > With that knowledge and a debugger I found the reason: a buffer
> > overflow in Paths_Init(). When started from cmd.exe giving a
> > relative path to the executable, psExecDir [1] will also only
> > contain a relative path. Notice how in [2] only just enough memory
> > is allocated for sDataDir to hold psExecDir, the path separator
> > ("\" under Windows) and BIN2DATADIR, which is "." for the Windows
> > build.
> > 
> > However, File_MakeAbsoluteName, called from [3], will in the end
> > write back the full, absolute filename into that buffer [4], thereby
> > overflowing it.  

Ouch.

> > Since Windows 10 has very good checkers for heap
> > corruption (e.g. by buffer overflow), it then almost immediately
> > terminates the offending process.  
> 
> I've always though Linux has better / more accessible ones, but
> strangely enough e.g. Valgrind doesn't complain about this.

The bug normally does not happen on Linux: Paths_InitExecDir() returns
an absolute path here since we can read the location with readlink().

> > Solution: Always allocate a sufficiently sized buffer
> > (FILENAME_MAX+1?) for sDataDir.  
> 
> I think FILENAME_MAX is supposed to include the terminating nil, as
> that's also specified part of the filename (given to Linux system
> calls).

Right.

> Attached is patch for you to test.

Please also check the return code of malloc for NULL (or use
Str_Alloc() to get the memory), otherwise static analyzers will
complain again.

 Thanks,
  Thomas