Обсуждение: question on how to correctly communicate with external library functions which return malloc()'ed strings

Hi!

i'm using a module for postgresql (8.2 and 8.3) which links with a
3rd-party library and calls a function from there which returns a
malloc()'ed string.

[it's a libunac, and unac_string() but the question is general]

there was a pg_unac-8.2.tar.gz tarball distributed on the net, which has
the following implementation:

=============================================
Datum unac(PG_FUNCTION_ARGS) {
 text *str = PG_GETARG_TEXT_P(0); text *result;  int tlen, nlen, rsize; char *tstr, *nstr, *rptr;  tlen = VARSIZE(str)
-VARHDRSZ; tstr = (char *) palloc(tlen + 1); memcpy(tstr, VARDATA(str), tlen); tstr[tlen] = '\0';
 
 nstr = NULL; nlen = 0; unac_string("UTF-8", tstr, strlen(tstr), &nstr, &nlen);
 /* It may happen that unac_string returns NULL, because iconv  * can't translate the input string. In this case we
output * the string as it is. */ if (nstr == NULL) nstr = tstr;  rsize = strlen(nstr) + VARHDRSZ; result = (text *)
palloc(rsize);rptr =  VARDATA(result); memcpy(rptr, nstr, rsize - VARHDRSZ); SET_VARSIZE(result, rsize);
PG_RETURN_TEXT_P(result);
 
}
=============================================

clearly there's a memory leak here: nstr never gets free()'d:
its value is copied to a palloc()-ated string but nstr itself is
never free()'d.

this call to palloc() appears to be necessary because one cannot just
pass malloc()-created nstr as a result of a function - there will be a
segfault, as far as i remember; therefore one needs to copy nstr to a
palloc()-created string and then to free() nstr (which is missing in the
above code).

i had to modify that function to remove the memory leak and to add some
more functionality i needed.

in a simplified form, it looks like:

============================================= /* tstr is a palloc()'ed string */ size_t nlen = 0; char *nstr = NULL;
unac_string("UTF-8",tstr, strlen(tstr), &nstr, &nlen); if (nstr == NULL) {   nstr = strdup(tstr); } pfree(tstr); result
=(text *) palloc(strlen(nstr) + VARHDRSZ); memcpy(VARDATA(result), nstr, strlen(nstr));
 
#if PG_VERSION_NUM >= 80300 SET_VARSIZE(result, strlen(nstr) + VARHDRSZ);
#else VARATT_SIZEP(result) = strlen(nstr) + VARHDRSZ;
#endif free(nstr); PG_RETURN_TEXT_P(result);
=============================================

it worked fine with postgresql 8.2; with 8.3 it started segfaulting, and
it appeared that the reason is because in postgresql 8.3, the "free" is
a macro defined in
snowball/header.h:
#define free(a) pfree(a)

i solved it by adding these lines:

#ifdef free
#warning undefining free
#undef free
#endif
#include <stdlib.h>

but i want to ask you, postgresql hackers, on how we are supposed to
work with external library functions which return malloc()'ed strings?

should we create palloc()'ed copy and free() the original string?
then, is it correct to have that "#define free(a) pfree(a)" in
snowball/header.h?

Best,
v.

On Thu, Apr 10, 2008 at 07:28:29PM -0700, Vladimir Volovich wrote:
> it worked fine with postgresql 8.2; with 8.3 it started segfaulting, and
> it appeared that the reason is because in postgresql 8.3, the "free" is
> a macro defined in
> snowball/header.h:
> #define free(a) pfree(a)

It does seem wrong. Do you include that header file explicitly? Because
it shouldn't be necessary.

> #ifdef free
> #warning undefining free
> #undef free
> #endif
> #include <stdlib.h>
>
> but i want to ask you, postgresql hackers, on how we are supposed to
> work with external library functions which return malloc()'ed strings?

See if you can avoid including that header altogether...

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> Please line up in a tree and maintain the heap invariant while
> boarding. Thank you for flying nlogn airlines.

"MvO" == Martijn van Oosterhout writes:
>> it worked fine with postgresql 8.2; with 8.3 it started segfaulting,>> and it appeared that the reason is because in
postgresql8.3, the>> "free" is a macro defined in snowball/header.h: #define free(a)>> pfree(a)
 
MvO> It does seem wrong. Do you include that header file explicitly?MvO> Because it shouldn't be necessary.

i needed some prototypes from snowball in the same source file, so i
included snowball/header.h; it appears that it is sufficient to use #include <snowball/libstemmer/api.h>
instead of #include <snowball/header.h>

but please remove the "#define free" from snowball/header.h because, as
you said, it's wrong.

Best,
v.

Vladimir Volovich <vvv@vsu.ru> writes:
> but please remove the "#define free" from snowball/header.h because, as
> you said, it's wrong.

It's not wrong and it won't be removed.  Please note the header comment
in that file:
* NOTE: this file should not be included into any non-snowball sources!
        regards, tom lane

"TL" == Tom Lane writes:
TL> It's not wrong and it won't be removed.  Please note the headerTL> comment in that file:
TL>  * NOTE: this file should not be included into any non-snowballTL> sources!

ok, i'll just include snowball/libstemmer/api.h or snowball/libstemmer/header.h instead.

in postgresql 8.2 there was no such "#define free" in snowball/header.h,
and a change in 8.3 introduced breakage; i should have looked in the new
snowball/header.h comment prior to complaining.

thanks.

Best,
v.