Returning a function argument list from a Custom function results in error

[drewwells]

This code for example should reproduce the error. I'm doing this in Go, but this example should also throw the same error.

union Sass_Value* CustomFunc( union Sass_Value* s_args, void* cookie ) {
    return s_args;
}

Error:

malloc: *** error for object 0x451a590: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
SIGABRT: abort
PC=0x7fff98e2c282
signal arrived during cgo execution

[mgreter]

Not sure if we really can call this a bug, since you should make a copy of that Sass_Value. How do you know what the lifespan of s_args is? You only know for sure, that it will be valid for the time that your function is executed. The return value itself will be freed later, since you give ownership of it back to libsass. So by returning the same instance, you tell libsass to free it twice (That's what I guess is happening). We should probably have made the input argument a const, but I'm not a native c++ developer, so I have to make educated guesses here!

[drewwells]

I think you hit the nail on the head.
On Fri, Dec 5, 2014 at 7:30 PM Marcel Greter notifications@github.com
wrote:

Not sure if we really can call this a bug, since you should make a copy of
that Sass_Value. How do you know what the lifespan of s_args is? You only
know for sure, that it will be valid for the time that your function is
executed. The return value itself will be freed later, since you give
ownership of it back to libsass. So by returning the same instance, you
tell libsass to free it twice (That's what I guess is happening). We should
probably have made the input argument a const, but I'm not a native c++
developer, so I have to make educated guesses here!

—
Reply to this email directly or view it on GitHub
#689 (comment).

[mgreter]

I still think this is somewhat a valid bug report. But I don't know what's the best way to address this (should the arg be const?). I guess a sass_clone_value would be handy in this case (whatever the reason is to return the input value back). To be honest, it would not be the simplest function to implement (as it is recursive ... but feasible).

Can this issue be closed beside the mentioned improvements? If so, feel free to open separate issues!

[drewwells]

Yeah I think it should. It would be far better that this variable is freed
than to expect the implementation code to free it.
On Fri, Dec 5, 2014 at 9:00 PM Marcel Greter notifications@github.com
wrote:

I still think this is somewhat a valid bug report. But I don't know what's
the best way to address this (should the arg be const?). I guess a
sass_clone_value would be handy in this case (whatever the reason is to
return the input value back). To be honest, it would not be the simplest
function to implement (as it is recursive ... but feasible).

Can this issue be closed beside the mentioned improvements? If so, feel
free to open separate issues!

—
Reply to this email directly or view it on GitHub
#689 (comment).

[mgreter]

I don't really understand your last comment. The thing is that implementors do not have to free anything, but we expect you to create a new variable to return in custom functions. Adding const seems to only issue a warning for this case (which is better than nothing IMO). Not sure if we can do much more, other than documenting it properly and relying on implementors to handle it correctly.

I also added a sass_clone_value function to the API, so you could do:

union Sass_Value* CustomFunc( const union Sass_Value* s_args, void* cookie ) {
    return sass_clone_value(s_args);
}

[drewwells]

Sorry I was typing on a cell phone. I was trying to say that I agree with your implementation. It's better that libsass frees these variables by default.