Hum... I don't rememeber of this code... Where it is ?
(copying pike-devel, as I'm thinking there are potential pike issues at play here...)
This code is from caudium.c; make_tag_attributes() and encode_mapping (). But I'm not convinced it's directly that code. Any arrays I created and passed to f_replace() cause the crash. If you create the arrays, push them onto the stack, skip calling f_replace() then pop them back off, the problem goes away, without memory leak. I also don't get the problem if I pass all strings to f_replace(), but that's a different set of replace code anyhow. There's a bunch of string builder code in f_replace, and string builder is also where things die later, so it makes me think there's something bad happening in there, though I realize that looks can be deceiving.
For those in pike-devel land that might have a moment to comment, the code in question seems to be:
static struct mapping *encode_mapping(struct mapping *mapping2encode, int encode_type) { struct array *indices, *values; struct mapping *result; struct pike_string *key = NULL, *val = NULL, *tmp = NULL; int i, j, k, do_replace; int size;
indices = mapping_indices(mapping2encode); values = mapping_values(mapping2encode); size = (unsigned)indices->size; result = allocate_mapping(size); if(result == NULL) Pike_error("Can't allocate result mapping\n");
/* encode any key/value pair in the mapping */ for(i = 0; i < size; i++) { if(indices->real_item[i].type != T_STRING || values->real_item[i].type != T_STRING) continue; for(j = 0; j < 2; j++) { if(j == 0) tmp = indices->real_item[i].u.string; if(j == 1) tmp = values->real_item[i].u.string; /* let's see whether we have anything to encode */ do_replace = 0; if(encode_type == 1) for (k = 0; k < XML_UNSAFECHARS_SIZE; k++) { if (memchr(tmp->str, xml_unsafechars[k][0], tmp->len)) { do_replace = 1; break; } } if(encode_type == 0) for (k = 0; k < HTML_UNSAFECHARS_SIZE; k++) { if (memchr(tmp->str, html_unsafechars[k][0], tmp->len)) { do_replace = 1; break; } }
if (do_replace) { push_string(tmp); if(encode_type == 1) { push_text("<"); push_text(">"); push_text("&"); push_text("""); push_text("'"); push_text("\000"); f_aggregate(6);
push_text("<"); push_text(">"); push_text("&"); push_text("""); push_text("'"); push_text("�"); f_aggregate(6);
} if(encode_type == 0) { push_text("""); f_aggregate(1); push_text("""); f_aggregate(1);
} pop_n_elems(2); // f_replace(3); if(j == 0) copy_shared_string(key, Pike_sp[-1].u.string); if(j == 1) copy_shared_string(val, Pike_sp[-1].u.string); pop_stack(); } else { if(j == 0) copy_shared_string(key, tmp); if(j == 1) copy_shared_string(val, tmp); } } mapping_string_insert_string(result, key, val); } free_array(indices); free_array(values); return result; }
the current code that's causing problems is:
http://cvsweb.caudiumforge.net/cgi-bin/cvsweb.cgi/caudium/src/cmods/ _Caudium/caudium.c?rev=1.160;content-type=text%2Fx-cvsweb- markup;cvsroot=caudium
Previously, the two arrays that would be passed to f_replace() were premade, and a copy was just passed in (though it's not clear that they needed to be copied). This all started when I went looking for a leak, and added the free_array() calls that you see in f_make_tag_attributes() and encode_mapping(). Apparently the code doesn't crash if you don't free those arrays (indices and values), and that's the original author wasn't freeing them, but I don't think it should be necessary to endure a memory leak just for the sake of the code not crashing. I can run this code for hours in an endless loop and not have problems or leaks, but caudium will die on the first request, usually somewhere inside Parser.HTML... I posed a backtrace to the main list a few days ago, but it had me totally confused.
It might be worth mentioning that I had a problem with a similar end result (bus errors coming from malloc() by way of string builder code) with some code I was writing, but that didn't have anything to do with this caudium c module. Unfortunately, I can't produce that code because I didn't check it in anywhere, and resolved the problem by rewriting the pike code that seemed to trigger it. The similarity of the two results makes me think that perhaps there's a problem somewhere...
Any suggestions?
Bill
The real question is why this code is there in the first place. Isn't it an order of magnitude slower than doing it in Pike? I sent a replacement C function to the caudium list (?) a few years back.
Anyways, isn't tmp going to underflow as you push it (not gaining reference) and then pop it through calling f_replace (loosing one reference)? Perhaps ref_push_string(tmp) works better?
Anyways, isn't tmp going to underflow as you push it (not gaining reference) and then pop it through calling f_replace (loosing one reference)? Perhaps ref_push_string(tmp) works better?
out of curiosity, exactly what causes a ref to be lost when calling (f_replace, for example?) is it the fact that f_replace pops the string when it's done with it, or is there some other mechanism?
I have a general understanding of how ref counting works, but was just curious from an implimentation standpoint.
Bill
Yes, is is becase the stack is popped that the refcount is decremented; after all, the stack is no longer referencing the string. Note that push_string() does _not_ increase the refcount, so you can only use this function when you already have a reference that you wish to "donate" to the stack. If you want to both push the string, and keep your own reference, you need to use ref_push_string.
pike-devel@lists.lysator.liu.se