Everything

php

RETURN_RT_STRING() and family leakage

php.internals

Sara Golemon

20 years ago

I was going through failing tests today and noticed some which failed
because of memory leaks using code like the following:

RETURN_RT_STRING(s, 0);
}

For non-unicode mode this is functionally identical to:

RETURN_STRING(s, 0);
}

However for unicode mode, this binary value is translated to unicode using
(effectively) this:

{
UChar *u;
int u_len;
UErrorCode status = U_ZERO_ERROR;
zend_convert_to_string(ZEND_U_CONVERTER(UG(runtime_encoding_conv)),
&u, &u_len, s, strlen(s), &status);
RETURN_UNICODE(u, 0);
}
}

The trouble with this, is that the original s value winds up getting leaked.
I noticed a few other spots in the source where this is handled as such:

RETVAL_RT_STRING(s, 0);
if (UG(unicode)) {
efree(s);
}
return;
}

Which is, of course, perfectly valid, but it feels a bit cludgy to me as
it's a little inconsistent with the normal RETURN_STRING()/RETURN_UNICODE()
semantics when it comes to "giving away" the variable.  It's also a problem
for the RETURN_RT_STRING(s, 0); usage in general as there's no point after
the macro to free the original var.

I see two solutions:

(1) Modify the RETURN_RT_STRING(L)() macros to the following:

#define RETURN_RT_STRING(t, duplicate) \
{ RETVAL_RT_STRING(t, duplicate); if (duplicate && UG(unicode)) efree(t);
return; }

(2) Modify ZVAL_U_STRINGL() to:

#define ZVAL_U_STRINGL(conv, z, s, l, duplicate, auto_free) \
if (UG(unicode)) { \
UErrorCode status = U_ZERO_ERROR; \
UChar *u_str; \
int u_len; \
zend_convert_to_unicode(conv, &u_str, &u_len, s, l, &status); \
if (auto_free && !duplicate) { \
efree(s); \
} \
ZVAL_UNICODEL(z, u_str, u_len, 0); \
} else { \
char *__s=(s); int __l=l;   \
Z_STRLEN_P(z) = __l;        \
Z_STRVAL_P(z) = (duplicate?estrndup(__s, __l):__s); \
Z_TYPE_P(z) = IS_STRING;    \
}

Along with changes to the (ZVAL|RETVAL)_RT_STRING() macros: to support the
additional auto_free option (with RETURN_* assuming auto_free=1).

The first option solves the one real problem with the current implementation
by making the uncatchable RETURN_RT_STRING() macros free the original char*
as needed, whereas the latter takes that work away from the calling scope at
the cost of adding to the proto and requiring more work to go back and clean
up existing uses.

Thoughts?

-Sara

Dmitry Stogov

20 years ago

Hi Sara,

Your first solution will not work. String passed to ZVAL_RETURN_RT_STRING()
may be not allocated by emalloc().

The second solution will work.

ZVAL_RETURN_RT_STRINGL(str, len, duplicate) -> ZVAL_RETURN_RT_STRINGL(str,
len, duplicate, auto_free)

3) It is possible to reuse "duplicate" argument
0 - don't duplicate
1 - duplicate
2 - duplicate and free

Thanks. Dmitry.

Sara Golemon

20 years ago

> Your first solution will not work. String passed to
> ZVAL_RETURN_RT_STRING()
> may be not allocated by emalloc().
>

duplicate should only ever be set to 0 on this (or any of the macros) when
the string *is* allocated with emalloc.  Otherwise the enegine would get in
trouble freeing it later on.  I did just notice that I paste the wrong
version into my post though.... it should have been: if (!duplicate &&
UG(unicode) ...

#define RETURN_RT_STRING(t, duplicate) \
{ RETVAL_RT_STRING(t, duplicate); if (!duplicate && UG(unicode)) efree(t);
return; }

> The second solution will work.
>
> ZVAL_RETURN_RT_STRINGL(str, len, duplicate) ->
> ZVAL_RETURN_RT_STRINGL(str, len, duplicate, auto_free)
>

There's one other we came up with:

Leave existing protos as is, having them assume auto-free when duplicate==0
(There is no issue when duplicate==1).
Create an ad=ditional set of macros: (ZVAL|RETVAL)_RT_STRINGL_NOFREE(str,
len) to be used when duplication (for the sake of owning the buffer) is not
needed (because it's emalloc'd), but where (str) should not be freeded even
in the eventuality that it's converted into a new buffer as unicode
contents.

This gives that edge 10% the ability to reuse (str) after populating it into
the zval.  A RETURN variant would be silly here as
RETURN_RT_STRING_NOFREE(str) would be guaranteed to leak in unicode mode.
(It converts into a new buffer then abandons the old one).

> 3) It is possible to reuse "duplicate" argument
> 0 - don't duplicate
> 1 - duplicate
> 2 - duplicate and free
>

Andrei and I tossed this around last night (and actually it's "don't
duplicate and free" since the logic leading to the need for an auto_free
assumes that the original string should not have been copied but the unicode
conversion demanded that it was).  The trouble with this approach is that
it's terribly inconsistent with other ZVAL/RETVAL/RETURN macros in use
everywhere else.  e.g. duplicate has always been a binary value, not a
trinary one.

-Sara

Dmitry Stogov

20 years ago

Hi Sara,

> -----Original Message-----
> From: Sara Golemon [mailto:pollita@php.net]
> Sent: Monday, April 03, 2006 9:03 PM
> To: "Dmitry Stogov"
> Cc: internals@lists.php.net
> Subject: Re: [PHP-DEV] RETURN_RT_STRING() and family leakage
>
>
> > Your first solution will not work. String passed to
> > ZVAL_RETURN_RT_STRING()
> > may be not allocated by emalloc().
> >
> duplicate should only ever be set to 0 on this (or any of the
> macros) when
> the string *is* allocated with emalloc.  Otherwise the
> enegine would get in
> trouble freeing it later on.

No. :(
You can use ZVAL_RT_STRING(&fname, "strlen", 0), then call
zend_call_function(&fname) and do not destroy fname.

Thanks. Dmitry.

Sara Golemon

20 years ago

>> duplicate should only ever be set to 0 on this (or any of the
>> macros) when
>> the string *is* allocated with emalloc.  Otherwise the
>> enegine would get in
>> trouble freeing it later on.
>
> No. :(
> You can use ZVAL_RT_STRING(&fname, "strlen", 0), then call
> zend_call_function(&fname) and do not destroy fname.
>

Doi, of course, good point...

Okay, then the options are:

(1) Assume auto_free for RETURN_RT_STRING(s,0) specifically as in this case,
my statement above is valid since the engine IS going to try it eventually.
Leave all other macros alone and make the calling scope handle the original
string with the existing if (UG(unicode)) efree(s);   Not my favorite.

(2) Expand (ZVAL|RETVAL)_RT_STRING(s, 0) to include auto_free argument.
This coule be done in conjunction with (1) or in place of it.

(3) Overload duplicate argument to include (should I auto-free?) logic.

(4) Duplicate the macros to one auto-free, and one non-auto-free version.

I like the #1/#2 combo personally.

-Sara

Dmitry Stogov

20 years ago

I prefer (2) or (3).
(3) requires less changes.

#define S_DUPLICATE	(1<<0)
#define S_AUTO_FREE (1<<1)

#define ZVAL_U_STRINGL(conv, z, s, l, flags) \
if (UG(unicode)) { \
UErrorCode status = U_ZERO_ERROR; \
UChar *u_str; \
int u_len; \
zend_convert_to_unicode(conv, &u_str, &u_len, s, l,
&status); \
ZVAL_UNICODEL(z, u_str, u_len, 0); \
if (flags & S_AUTO_FREE) {efree(s);} \
} else { \
char *__s=(s); int __l=l;	\
Z_STRLEN_P(z) = __l;	    \
Z_STRVAL_P(z) = ((flags & S_DUPLICATE)?estrndup(__s,
__l):__s);	\
Z_TYPE_P(z) = IS_STRING;    \
}

And then we need to change only calls those rally leak.

RETURN_RT_STRING(str, 1) -> RETURN_RT_STRING(str, S_DUPLICATE | S_AUTO_FREE)

Thanks. Dmitry.

Sara Golemon

20 years ago

Here's what I plan to commit unless I hear otherwise.

I went with ZSTR_ as the prefix for the constants as S_ isn't terribly
unique.  If anyone has a better suggestion, now is the time to voice it.

-Sara

----- Original Message -----
From: Dmitry Stogov
To: 'Sara Golemon'
Cc: internals@lists.php.net
Sent: Monday, April 03, 2006 10:02 PM
Subject: RE: [PHP-DEV] RETURN_RT_STRING() and family leakage


I prefer (2) or (3).
(3) requires less changes.

#define S_DUPLICATE (1<<0)
#define S_AUTO_FREE (1<<1)

#define ZVAL_U_STRINGL(conv, z, s, l, flags) \
if (UG(unicode)) { \
UErrorCode status = U_ZERO_ERROR; \
UChar *u_str; \
int u_len; \
zend_convert_to_unicode(conv, &u_str, &u_len, s, l,
&status); \
ZVAL_UNICODEL(z, u_str, u_len, 0); \
if (flags & S_AUTO_FREE) {efree(s);} \
} else { \
char *__s=(s); int __l=l; \
Z_STRLEN_P(z) = __l;     \
Z_STRVAL_P(z) = ((flags & S_DUPLICATE)?estrndup(__s,
__l):__s); \
Z_TYPE_P(z) = IS_STRING;    \
}

And then we need to change only calls those rally leak.

RETURN_RT_STRING(str, 1) -> RETURN_RT_STRING(str, S_DUPLICATE | S_AUTO_FREE)

Thanks. Dmitry.

Dmitry Stogov

20 years ago

Hi Sara,

All right. I haven't objections.
Also, please update ..._ASCII_STRING() macros family to use these flags.
They have exactly the same problem.

Thanks. Dmitry.