EtcTool crash

[spearx]

Hi guys, sometimes i have a crash in the EtcTool.

I reviewed the code and i found a char array with a bad size.

In the EtcTool.cpp in function ProcessCommandLineArguments
if (pstrOutputFilename[c] == ETC_PATH_SLASH) { c++; ptrOutputDir = new char[c]; strncpy(ptrOutputDir, pstrOutputFilename, c); ptrOutputDir[c] = '\0'; CreateNewDir(ptrOutputDir); break; }

The ptrOutputDir variable has a bad size, the must be c+1, i changed my code to:

if (pstrOutputFilename[c] == ETC_PATH_SLASH) { c++; ptrOutputDir = new char[c+1]; strncpy(ptrOutputDir, pstrOutputFilename, c); ptrOutputDir[c] = '\0'; CreateNewDir(ptrOutputDir); break; }

And now all run right.

Thanks,

Ruben

[mainroach]

Can you provide your input argument that causes the crash in ptrOutputDir ?

[jclee]

For what it's worth, I'm encountering the same bug when running etctool under Ubuntu Precise. It's a memory error, so reproducing it in any given environment is probably going to be difficult. But in my specific case, I've observed the failure as a tripped malloc() assert for certain output path formats. For example, this output path containing a directory name of exactly 23 characters reproduces the crash for me, while directories of other lengths (tried 4-36) do not:

./etctool data/00007.png -output out_xxxxxxxxxxxxxxxxxxx/00007.ktx
etctool: malloc.c:2372: sysmalloc: Assertion `(old_top == (((mbinptr) (((char *) &((av)->bins[((1) - 1) * 2])) - __builtin_offsetof (struct malloc_chunk, fd)))) && old_size == 0) || ((unsigned long) (old_size) >= (unsigned long)((((__builtin_offsetof (struct malloc_chunk, fd_nextsize))+((2 *(sizeof(size_t))) - 1)) & ~((2 *(sizeof(size_t))) - 1))) && ((old_top)->size & 0x1) && ((unsigned long) old_end & pagemask) == 0)' failed.

A gdb stacktrace shows the failure occurs in lodepng_realloc() at third_party/lodepng/lodepng.cpp:73, although I'm pretty sure there's nothing wrong with lodepng.cpp -- it's just fallout from the buffer overrun in EtcTool.cpp.

The corruption is reliably detected by valgrind for any input:

valgrind etctool data/00007.png -mipmaps 100 -format RGBA8 -effort 50 -output out/00007.ktx
[...]
      ==47421== Invalid write of size 1
      ==47421==    at 0x434BC6: Commands::ProcessCommandLineArguments(int, char const**) (EtcTool.cpp:656)
      ==47421==    by 0x4335D2: main (EtcTool.cpp:139)
      ==47421==  Address 0x5c3a178 is 0 bytes after a block of size 24 alloc'd
      ==47421==    at 0x4C2B800: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
      ==47421==    by 0x434B97: Commands::ProcessCommandLineArguments(int, char const**) (EtcTool.cpp:654)
      ==47421==    by 0x4335D2: main (EtcTool.cpp:139)
      ==47421== 
      ==47421== Invalid read of size 1
      ==47421==    at 0x58C0943: vfprintf (vfprintf.c:1661)
      ==47421==    by 0x58E58FA: vsprintf (iovsprintf.c:42)
      ==47421==    by 0x58C9506: sprintf (sprintf.c:32)
      ==47421==    by 0x435176: CreateNewDir(char const*) (EtcTool.cpp:821)
      ==47421==    by 0x434BD4: Commands::ProcessCommandLineArguments(int, char const**) (EtcTool.cpp:657)
      ==47421==    by 0x4335D2: main (EtcTool.cpp:139)
      ==47421==  Address 0x5c3a178 is 0 bytes after a block of size 24 alloc'd
      ==47421==    at 0x4C2B800: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
      ==47421==    by 0x434B97: Commands::ProcessCommandLineArguments(int, char const**) (EtcTool.cpp:654)
      ==47421==    by 0x4335D2: main (EtcTool.cpp:139)
[...]

Per @spearx 's suggestion, I am able to fix both the valgrind output and the malloc assertion by expanding the buffer by one to accommodate the null terminator, on the following line:

https://github.com/google/etc2comp/blob/e2e733c/EtcTool/EtcTool.cpp#L654

[bonizz]

I agree with @spearx and @jclee, it looks like an off-by-one error.

The allocated size of the buffer is of size c, but ptrOutputDir[c] is one past the size of the buffer.
ptrOutputDir[c] = '\0'; writes a 0 past the allocated buffer.

Either ptrOutputDir[c-1] = '\0'; (to trim the trailing slash) or ptrOutputDir = new char[c+1]; should fix.

Edit: test case is to have an output file with a slash (dir separator) in it