Set the correct pointer value in 'ecma_copy_ecma_string' function. #1010
Conversation
JerryScript-DCO-1.0-Signed-off-by: Robert Sipka rsipka.uszeged@partner.samsung.com
|
Please add some regression test for this. |
LaszloLango
added
bug
| @@ -422,7 +422,7 @@ ecma_copy_ecma_string (ecma_string_t *string_desc_p) /**< string descriptor */ | |||
| ecma_string_heap_header_t *new_data_p = (ecma_string_heap_header_t *) mem_heap_alloc_block (data_size); | |||
| memcpy (new_data_p, data_p, data_p->size + sizeof (ecma_string_heap_header_t)); | |||
|
|
|||
| ECMA_SET_NON_NULL_POINTER (new_str_p->u.collection_cp, data_p); | |||
| ECMA_SET_NON_NULL_POINTER (new_str_p->u.collection_cp, new_data_p); | |||
There was a problem hiding this comment.
Quick question: is this the only issue in this code block? I'm not familiar with this function but we have *new_str_p = *string_desc_p; above. That copies over refs_and_container, hash, and u.collection_cp. The latter will be overwritten (fixed in this patch), hash does not change. What about the reference count? Is that OK not to (re)set?
There was a problem hiding this comment.
Just check the comment at the beginning:
@return pointer to copy of ecma-string with reference counter set to 1
There was a problem hiding this comment.
So probably copying the refcounter is also a bug. I had a feelewng that this is an untested code path. That is why we need a reftest.
There was a problem hiding this comment.
I see the comment. But where is it set to 1 in this code path?
There was a problem hiding this comment.
I agree this is probably another bug. Good catch. We definitely need a reftest for this.
There was a problem hiding this comment.
I'm not sure that it is possible to construct a regression test case that can catch the refcount issue... It seems to me that this will be "just" eating up memory but not cause any semantic error. I'd be glad to be proven wrong though.
(The pointer issue is different. That might be possible to catch.)
There was a problem hiding this comment.
Here the problem is that there is a reference to the old string. A regression test would look like this:
var a = "string";
var b = [];
for (i = 0; i < 10000; i++) b[i] = a;
gc();
for (i = 0; i < 9000; i++) b[i] = null;
gc(); // could be a crash
|
Is this issue still valid? |
JerryScript-DCO-1.0-Signed-off-by: Robert Sipka rsipka.uszeged@partner.samsung.com