Potential memory corruption bug from undefined behaviour

[leecbaker]

While debugging #56, my tools picked up the following bug. strcpy()'s two parameters' ranges should not overlap; when they overlap, this is undefined behavior (reference), perhaps resulting in memory corruption.

One way to fix this is to use memmove().

This happens here: https://github.com/mSparks43/XPlane-11-AutoATC-plugin/blob/27e9f5ebb6d0621a76fe2fb87d2f960db1c3da29/src/jvm.cpp#L380

Here's the tool output. Once we know what's happening here, probably don't need to read through this.

=================================================================
==49630==ERROR: AddressSanitizer: strcpy-param-overlap: memory ranges [0x7ffeefbfc460,0x7ffeefbfc496) and [0x7ffeefbfc460, 0x7ffeefbfc496) overlap
2020-04-03 19:53:50.479617+1200 atos[49636:953645] examining /Users/USER/Desktop/*/X-Plane.app/Contents/MacOS/X-Plane [49630]
    #0 0x6aee1 in wrap_strcpy (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x5aee1)
    #1 0x8ae52f9e in JVM::trim(char*) jvm.cpp:380
    #2 0x8ae55197 in JVM::parse_config(char*) jvm.cpp:583
    #3 0x8ae3d818 in XPluginStart AutoATCv0.8.7.cpp:111
    #4 0x950405e6 in LoadOnePlugin(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, int, int) (XPLM:x86_64+0x85e6)
    #5 0x9503f783 in LoadFatPlugin(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, int) (XPLM:x86_64+0x7783)
    #6 0x95040149 in XPLMFindAndLoadPlugins(char const*) (XPLM:x86_64+0x8149)
    #7 0x9503ef45 in XPLMInitializePlugins (XPLM:x86_64+0x6f45)
    #8 0x100f7b2c4 in XPPInitializePlugins(char const*) (X-Plane:x86_64+0x100f7b2c4)
    #9 0x1000948b3 in MACIBM_init(int, char const* const*) (X-Plane:x86_64+0x1000948b3)
    #10 0x100bcbd3c in init_sim(int, char const* const*) (X-Plane:x86_64+0x100bcbd3c)
    #11 0x100bcc3fa in main (X-Plane:x86_64+0x100bcc3fa)
    #12 0x7fff6ac9dcc8 in start (libdyld.dylib:x86_64+0x1acc8)

Address 0x7ffeefbfc460 is located in stack of thread T0 at offset 4384 in frame
    #0 0x8ae54d3f in JVM::parse_config(char*) jvm.cpp:555

  This frame has 5 object(s):
    [32, 2080) 'buff' (line 557)
    [2208, 4256) 'name' (line 572)
    [4384, 6432) 'value' (line 572) <== Memory access at offset 4384 is inside this variable
    [6560, 8864) 'def' (line 600)
    [8992, 9016) 'agg.tmp'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
Address 0x7ffeefbfc460 is located in stack of thread T0 at offset 4384 in frame
    #0 0x8ae54d3f in JVM::parse_config(char*) jvm.cpp:555

  This frame has 5 object(s):
    [32, 2080) 'buff' (line 557)
    [2208, 4256) 'name' (line 572)
    [4384, 6432) 'value' (line 572) <== Memory access at offset 4384 is inside this variable
    [6560, 8864) 'def' (line 600)
    [8992, 9016) 'agg.tmp'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: strcpy-param-overlap (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x5aee1) in wrap_strcpy
2020-04-03 19:53:50.619710+1200 X-Plane[49630:953426] =================================================================
2020-04-03 19:53:50.619750+1200 X-Plane[49630:953426] ==49630==ERROR: AddressSanitizer: strcpy-param-overlap: memory ranges [0x7ffeefbfc460,0x7ffeefbfc496) and [0x7ffeefbfc460, 0x7ffeefbfc496) overlap
2020-04-03 19:53:50.619766+1200 X-Plane[49630:953426]     #0 0x6aee1 in wrap_strcpy (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x5aee1)
2020-04-03 19:53:50.619780+1200 X-Plane[49630:953426]     #1 0x8ae52f9e in JVM::trim(char*) jvm.cpp:380
2020-04-03 19:53:50.619794+1200 X-Plane[49630:953426]     #2 0x8ae55197 in JVM::parse_config(char*) jvm.cpp:583
2020-04-03 19:53:50.619800+1200 X-Plane[49630:953426]     #3 0x8ae3d818 in XPluginStart AutoATCv0.8.7.cpp:111
2020-04-03 19:53:50.619807+1200 X-Plane[49630:953426]     #4 0x950405e6 in LoadOnePlugin(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, int, int) (XPLM:x86_64+0x85e6)
2020-04-03 19:53:50.619816+1200 X-Plane[49630:953426]     #5 0x9503f783 in LoadFatPlugin(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, int) (XPLM:x86_64+0x7783)
2020-04-03 19:53:50.619830+1200 X-Plane[49630:953426]     #6 0x95040149 in XPLMFindAndLoadPlugins(char const*) (XPLM:x86_64+0x8149)
2020-04-03 19:53:50.619837+1200 X-Plane[49630:953426]     #7 0x9503ef45 in XPLMInitializePlugins (XPLM:x86_64+0x6f45)
2020-04-03 19:53:50.619843+1200 X-Plane[49630:953426]     #8 0x100f7b2c4 in XPPInitializePlugins(char const*) (X-Plane:x86_64+0x100f7b2c4)
2020-04-03 19:53:50.619850+1200 X-Plane[49630:953426]     #9 0x1000948b3 in MACIBM_init(int, char const* const*) (X-Plane:x86_64+0x1000948b3)
2020-04-03 19:53:50.619858+1200 X-Plane[49630:953426]     #10 0x100bcbd3c in init_sim(int, char const* const*) (X-Plane:x86_64+0x100bcbd3c)
2020-04-03 19:53:50.619864+1200 X-Plane[49630:953426]     #11 0x100bcc3fa in main (X-Plane:x86_64+0x100bcc3fa)
2020-04-03 19:53:50.619871+1200 X-Plane[49630:953426]     #12 0x7fff6ac9dcc8 in start (libdyld.dylib:x86_64+0x1acc8)
2020-04-03 19:53:50.619878+1200 X-Plane[49630:953426]
2020-04-03 19:53:50.619884+1200 X-Plane[49630:953426] Address 0x7ffeefbfc460 is located in stack of thread T0 at offset 4384 in frame
2020-04-03 19:53:50.619890+1200 X-Plane[49630:953426]     #0 0x8ae54d3f in JVM::parse_config(char*) jvm.cpp:555
2020-04-03 19:53:50.619898+1200 X-Plane[49630:953426]
2020-04-03 19:53:50.619903+1200 X-Plane[49630:953426]   This frame has 5 object(s):
2020-04-03 19:53:50.619915+1200 X-Plane[49630:953426]     [32, 2080) 'buff' (line 557)
2020-04-03 19:53:50.619928+1200 X-Plane[49630:953426]     [2208, 4256) 'name' (line 572)
2020-04-03 19:53:50.619943+1200 X-Plane[49630:953426]     [4384, 6432) 'value' (line 572) <== Memory access at offset 4384 is inside this variable
2020-04-03 19:53:50.619953+1200 X-Plane[49630:953426]     [6560, 8864) 'def' (line 600)
2020-04-03 19:53:50.619959+1200 X-Plane[49630:953426]     [8992, 9016) 'agg.tmp'
2020-04-03 19:53:50.619966+1200 X-Plane[49630:953426] HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
2020-04-03 19:53:50.619979+1200 X-Plane[49630:953426]       (longjmp and C++ exceptions *are* supported)
2020-04-03 19:53:50.619987+1200 X-Plane[49630:953426] Address 0x7ffeefbfc460 is located in stack of thread T0 at offset 4384 in frame
2020-04-03 19:53:50.619994+1200 X-Plane[49630:953426]     #0 0x8ae54d3f in JVM::parse_config(char*) jvm.cpp:555
2020-04-03 19:53:50.620000+1200 X-Plane[49630:953426]
2020-04-03 19:53:50.620006+1200 X-Plane[49630:953426]   This frame has 5 object(s):
2020-04-03 19:53:50.620012+1200 X-Plane[49630:953426]     [32, 2080) 'buff' (line 557)
2020-04-03 19:53:50.620019+1200 X-Plane[49630:953426]     [2208, 4256) 'name' (line 572)
2020-04-03 19:53:50.620026+1200 X-Plane[49630:953426]     [4384, 6432) 'value' (line 572) <== Memory access at offset 4384 is inside this variable
2020-04-03 19:53:50.620033+1200 X-Plane[49630:953426]     [6560, 8864) 'def' (line 600)
2020-04-03 19:53:50.620041+1200 X-Plane[49630:953426]     [8992, 9016) 'agg.tmp'
2020-04-03 19:53:50.620048+1200 X-Plane[49630:953426] HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
2020-04-03 19:53:50.620055+1200 X-Plane[49630:953426]       (longjmp and C++ exceptions *are* supported)
2020-04-03 19:53:50.620063+1200 X-Plane[49630:953426] SUMMARY: AddressSanitizer: strcpy-param-overlap (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x5aee1) in wrap_strcpy
==49630==ABORTING

[mSparks43]

This shouldn't cause any issues. But may be worth fixing. It is a trim on a persistant string called at startup, taken from https://www.linuxquestions.org/questions/programming-9/read-parameters-from-config-file-file-parser-362188/

the string loaded into memory might be " hello " this trim tweeks the pointer such that referencing string only returns "hello" with probably "\0o " hanging off the end never used.

This, at least, is why it overlaps.

I'll have a look if there is a better trim implementation for C, I like how the mac tools check for things like this. Its Java equivalent is String.trim()

I'd quite like to replace the C++ plugin side strings with a utf8 implementation, using ANSI is terrible for i8n.

[leecbaker]

You can fix it really easily by replacing the strcpy() with memmove (s, s1, s2 - s1 + 1);. Should be the same thing, but memmove() permits overlapping regions. I think strcpy() (and memcpy()) don't allow overlapping inputs is because it removes a lot of complexity, and opens up some additional optimization possibilities.

I was using AddressSanitizer- this is an amazing tool for detecting all sorts of problems. It's available in GCC and Clang, and probably Windows as well. You just have to set up the right compilation flags.

If you want to use C++ strings here, it's easy to use boost::trim(), or you can make your own fairly easily. PlaneCommand uses this implementation, which is really not optimal but works correctly:

inline void string_trim(std::string & string) {
    while(false == string.empty() && std::isspace(static_cast<int>(string.front()))) {
        string.erase(string.begin());
    }
    while(false == string.empty() && std::isspace(static_cast<int>(string.back()))) {
        string.pop_back();
    }
}

[mSparks43]

It doesn't need to move any memory around because it is just changing the contents of single a fixed length array and the result is always at most the same length. probably easier to replace it with while (*s != '\0') { *s1 = *s; s1++; s++; } *s1 = '\0';

just in case the implementation is different.

[mSparks43]

added to next commit