C-API: memory management issues

[ckruse]

Hi,

Rodney just asked me about a problem he had with his emscripten libsass port: "this line fails, why?" The specific line was a cleanup line in context.cpp (line 121):

for (size_t i = 0; i < sources.size(); ++i) delete[] sources[i];

After checking I can reproduce this error. Valgrind says:

==1582== Mismatched free() / delete / delete []
==1582==    at 0x4C2BC50: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1582==    by 0x420E0D: Sass::Context::~Context() (in /home/ckruse/libsass/sassc/bin/sassc)
==1582==    by 0x40EFA2: sass_compile_context (in /home/ckruse/libsass/sassc/bin/sassc)
==1582==    by 0x40F738: sass_compile_data_context (in /home/ckruse/libsass/sassc/bin/sassc)
==1582==    by 0x409EEB: compile_stdin (sassc.c:86)
==1582==    by 0x40A793: main (sassc.c:282)
==1582==  Address 0x5c07bc0 is 0 bytes inside a block of size 32 alloc'd
==1582==    at 0x4C2C29E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1582==    by 0x409E07: compile_stdin (sassc.c:68)
==1582==    by 0x40A793: main (sassc.c:282)

After a bit of research it seems that you do not copy the strings the C API gets as an argument but destroy them with delete[]. This has to fail: memory blocks given by a C API is almost always reserved by malloc() and has to be deleted with free(). sassc seems to think so, too:

ctx = sass_make_data_context(source_string);
// […]
sass_delete_data_context(ctx);
free(source_string);

I don’t know enough about libsass internal handling of memory to provide a fix, but I guess that a simple copy using new in the exposed C API would fix this problem.

Regards,
CK

[ckruse]

Oh, I forgot to mention: I reproduced the problem using sassc and the input $foo:123px; .m { width:$foo; }

[mgreter]

Are you using latest master (3.2 beta)? Because there was a breaking change in this direction:

Change in sass_make_data_context - libsass now really takes memory ownership

The interface always was intended to be char*, but we messed that up and actually made a copy ourself (which would have meant the arg should have been const char* #869). The intention always was that implementors can pass the memory ownership to libsass, since we want to avoid a copy if possible. This basically just means that you probably have to provide a copy to libsass yourself.

// ctx = sass_make_data_context(source_string); // old
ctx = sass_make_data_context(strdup(source_string)); // new

BTW. we have sassc on the radar for 3.2, but I thought it was still working since CI didn't break?
sass/perl-libsass@3b34246#diff-5b390435e30d16e259818ad6b65d1dafL620

[ckruse]

The problem is not the missing strdup() call but the mix of malloc()/realloc() and delete. This is forbidden and leads to undefined behaviour. See the valgrind output. It doesn't make sense to expect a C API to get memory allocated by new since C doesn't know about it. It's a C++ feature. (Also this is not my source, I just discovered the problem)

Am 12. März 2015 23:00:30 MEZ, schrieb Marcel Greter [email protected]:

Are you using latest master (3.2 beta)? Because there was a breaking
change in this direction:

Change in sass_make_data_context - libsass now really takes memory
ownership

The interface always was intended to be char*, but we messed that up
and actually made a copy ourself (which would have meant the arg should
have been const char* #869). The intention always was that
implementors can pass the memory ownership to libsass, since we want to
avoid a copy if possible. This basically just means that you probably
have to provide a copy to libsass yourself.

// ctx = sass_make_data_context(source_string); // old
ctx = sass_make_data_context(strdup(source_string)); // new

---

Reply to this email directly or view it on GitHub:
#940 (comment)

Via mobile thus short. Sorry.

[mgreter]

Ok, so all you're saying is we should change delete[] sources[i] to free sources[i]? Since it is indeed a c string I agree, this would be the more logical thing to do. Seems like we got away with it so far ...

[ckruse]

Does sources only contain memory given by the C API? If yes then you should, yes. Regarding sassc working: yes it works, but it segfaults in the clean-up code.

[mgreter]

Sources is the main array/vector for all loaded content (libsass also adds the sources for files it loads via import to that vector, which should definitely be freed), We keep copies to those char* pointers in various locations, but they are only freed via sources. IMO the API makes sense now and is consistent, but as mentioned, it is a breaking change. Sassc hasn't been updated yet, and the segfault is pretty much expected, since now sassc and libsass will both try to free that memory! Either sassc has to pass ownership to libsass (and not free it on its own) or pass a copy to libsass!

[ckruse]

If the sources vector contains mixed memory (some allocated with
new, some allocated with malloc/realloc) you need to come up with
a strategy for that. You mustn’t use free on new allocated memory
and you mustnt usedeleteonmalloc` allocated memory. C++ memory
management differs from C memory management. This is important.

[am11]

What I know about this concept is that C++'s new/delete engages reference tracking and does a lot more then C's free(). However, C's malloc/free() are barebone memory operations, give more control and yet prone to SIGSEGVs etc. Mixing them lead to UB.

[rodneyrehm]

I've compiled current master with emscripten. It now fails because of the free. @ckruse can you reproduce that with sassc?

[am11]

I am getting a sigsegv on TravisCI on node-sass: https://travis-ci.org/sass/node-sass/jobs/54901826#L648
It doesn't tell much and it is not reproducible on Windows. I haven't probe the root cause because I am implementing custom functions feature atm, will report back if it is the same after the implementation.
Nonetheless, there is definitely something fishy about the memory management atm.

[ckruse]

@rodneyrehm nope, everything is fine now. Did you remove the free() in the wrapper?

[ckruse]

@rodneyrehm and if you give it strings allocated on the stack or the data segment (e.g. string literals) don't forget the strdup() call

[rodneyrehm]

there was no free() in the wrapper and yes, the strdup on my side fixed it. thx!