[perl #44499] Move cstrings to String Structure

[Chromatic]

# New Ticket Created by chromatic
# Please include the string: [perl #44499]
# in the subject line of all future correspondence about this issue.
# <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=44499 >


The string_from_cstring() function has a slight flaw, in that it has to
allocate a piece of memory and create a C-style string from a nice happy
STRING. It's the responsibility of the caller to discard the C string
appropriately.

This is tricky if that string immediately gets passed to real_exception() or
some other C function which does not return control; there's no chance to
free the string with string_cstring_free().

One solution is to use pointer tagging to mark pointers passed to
real_exception() and the like as freeable. However, that means scanning them
and getting their types right with regard to varargs and ow the hurting.

Another option is to cache the C string in the STRING structure itself in the
same way that Perl 5 caches IVs and NVs and PVs in the SV structure. (If
that doesn't mean anything to you, congratulations. I have this little
throbbing right behind my left eyeball just typing that.)

In other words, pin the lifetime of the malloced memory to the lifetime of a
garbage collected entity.

Obviously it's necessary to free the C string when destroying the string
(that's the point!) and it's necessary to free it and NULL out the pointer
when the string's contents change, but this would get rid of a memory leak
and, in the case of strings often converted to C strings, could improve
performance. (It also simplifies a problem in certain NCI calls that I
really don't want to get into.)

Though I hate to admit it, I think one of the pointers in the the PObj union
might work for this.

-- c

[Joshua Hoblitt]

Post by Chromatic
The string_from_cstring() function has a slight flaw, in that it has to
allocate a piece of memory and create a C-style string from a nice happy
STRING. It's the responsibility of the caller to discard the C string
appropriately.
This is tricky if that string immediately gets passed to real_exception() or
some other C function which does not return control; there's no chance to
free the string with string_cstring_free().

I hate to sound like the nag here but why don't we just not write code
in form 'real_exception(string_from_cstring(foo))'. Part of living with
dynamic memory allocating is dealing with the line bloat of shoving that
free() call in there.

-J

--

[Jerry Gay]

Post by Joshua Hoblitt

Post by Chromatic
The string_from_cstring() function has a slight flaw, in that it has to
allocate a piece of memory and create a C-style string from a nice happy
STRING. It's the responsibility of the caller to discard the C string
appropriately.
This is tricky if that string immediately gets passed to real_exception() or
some other C function which does not return control; there's no chance to
free the string with string_cstring_free().

I hate to sound like the nag here but why don't we just not write code
in form 'real_exception(string_from_cstring(foo))'. Part of living with
dynamic memory allocating is dealing with the line bloat of shoving that
free() call in there.

i'm not a great c programmer, so could you explain to me what code
should look like instead of what it shouldn't? here's the code at
src/pmc/delegate.pmc, line 51 as an example (copied here):

_class = string_to_cstring(interp, pmc->vtable->whoami);

real_exception(interp, NULL, E_LookupError,
"Can't find vtable method '%s' in class '%s'", meth, _class);

/* sorry, can't do this here. will it leak?
string_cstring_free(_class);
*/

~jerry

[Chromatic]

Post by Joshua Hoblitt
I hate to sound like the nag here but why don't we just not write code
in form 'real_exception(string_from_cstring(foo))'. Part of living with
dynamic memory allocating is dealing with the line bloat of shoving that
free() call in there.

To my knowledge, real_exception() doesn't return, so any subsequent free()
calls don't get called. If nothing catches the exception and Parrot exits,
that's fine. If something catches the call, it might or might not free the C
string.

-- c

[Mehmet Yavuz Selim Soyturk]

What about a real_exception variant that accepts STRING*'s as arguments?
On the other hand, as you said, there will be other functions which
don't return control (functions that call real_exception etc.). But
then char* is not the only problem, any malloced data can leak.
void f() {
void* leaked = malloc(n);
do_something(leaked); // does not come back
free(leaked);
}
void do_something(void*) {
something_else();
}
void something_else() {
if(condition) real_exception();
}

But you are 100% sure you are leaking in the real_exception case, of course.

[Mehmet Yavuz Selim Soyturk]

Post by Chromatic
# New Ticket Created by chromatic
# Please include the string: [perl #44499]
# in the subject line of all future correspondence about this issue.
# <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=44499 >
The string_from_cstring() function has a slight flaw, in that it has to
allocate a piece of memory and create a C-style string from a nice happy
STRING. It's the responsibility of the caller to discard the C string
appropriately.
This is tricky if that string immediately gets passed to real_exception() or
some other C function which does not return control; there's no chance to
free the string with string_cstring_free().
One solution is to use pointer tagging to mark pointers passed to
real_exception() and the like as freeable. However, that means scanning them
and getting their types right with regard to varargs and ow the hurting.
Another option is to cache the C string in the STRING structure itself in the
same way that Perl 5 caches IVs and NVs and PVs in the SV structure. (If
that doesn't mean anything to you, congratulations. I have this little
throbbing right behind my left eyeball just typing that.)
In other words, pin the lifetime of the malloced memory to the lifetime of a
garbage collected entity.
Obviously it's necessary to free the C string when destroying the string
(that's the point!) and it's necessary to free it and NULL out the pointer
when the string's contents change, but this would get rid of a memory leak
and, in the case of strings often converted to C strings, could improve
performance. (It also simplifies a problem in certain NCI calls that I
really don't want to get into.)
Though I hate to admit it, I think one of the pointers in the the PObj union
might work for this.
-- c

What about a real_exception variant that accepts STRING*'s as arguments?

On the other hand, as you said, there will be other functions which
don't return control (functions that call real_exception etc.). But
then char* is not the only problem, any malloced data can leak.

void f() {
void* leaked = malloc(n);
do_something(leaked); // does not come back
free(leaked);
}
void do_something(void*) {
something_else();
}
void something_else() {
if(condition) real_exception();
}

[NotFound]

On Wed, Aug 8, 2007 at 9:17 AM, via RT chromatic

Post by Chromatic
Another option is to cache the C string in the STRING structure itself in the
same way that Perl 5 caches IVs and NVs and PVs in the SV structure. (If
that doesn't mean anything to you, congratulations. I have this little
throbbing right behind my left eyeball just typing that.)

The attached patch uses a new function, Parrot_string_get_cstring, to
obtain a C string owned and cached by the parrot string, that lives
while the parrot string is not modified, the function
Parrot_string_free_cstring to delete the C string, intended only for
internal usage, adds the cstring_val member to the parrot string
struct to store the cached C string, and uses it in several places to
avoid string_to_cstring usage and some potential bugs related to it.

It uses string_to_cstring and string_cstring_free to implement these
new functions.

It also fills with garbage the C string in string_cstring_free, to
help catch errors of using a C string after freeing it.

[NotFound]

I tried to apply this patch to svn head, but it's different enough that
much of it will need to be rewritten.  If this is something worth
pursuing, I'm willing to reimplement and apply the patch (if NotFound
isn't interested).  If not, this ticket should be rejected.

Reject it. I'll take a look at the last patch to see if something is
still applicable.