Page 1 of 2
Bug in execute_from_str
Posted: Wed Sep 12, 2018 1:37 pm
by BramPeeters
Code: Select all
mp_obj_t execute_from_str(const char *str) {
nlr_buf_t nlr;
if (nlr_push(&nlr) == 0) {
mp_lexer_t *lex = mp_lexer_new_from_str_len(0/*MP_QSTR_*/, str, strlen(str), false);
mp_parse_tree_t pt = mp_parse(lex, MP_PARSE_FILE_INPUT);
mp_obj_t module_fun = mp_compile(&pt, lex->source_name, MP_EMIT_OPT_NONE, false);
mp_call_function_0(module_fun);
nlr_pop();
return 0;
}
Since mp_parse will free the lexer at the end, the lex->source_name argument of mp_compile is using memory that is not valid anymore.
Re: Bug in execute_from_str
Posted: Wed Sep 12, 2018 2:02 pm
by jickster
No.
lex->source_name is not a char * but a qstring.
Qstrings never get collected even if they’re only used ONCE. Qstrings are accumulated in RAM throughout the lifetime of the VM+heap (though there’s discussion to change that).
Sent from my iPhone using Tapatalk Pro
Re: Bug in execute_from_str
Posted: Wed Sep 12, 2018 2:12 pm
by BramPeeters
But 'lex' itself is collected, so you are dereferencing a memory address that no longer contains a valid value
You do not even get to the qstring anymore....
Re: Bug in execute_from_str
Posted: Wed Sep 12, 2018 2:13 pm
by jickster
BramPeeters wrote:But 'lex' itself is collected, so you are dereferencing a memory address that no longer contains a valid value
You do not even get to the qstring anymore....
Where do you see that lex is collected?
Sent from my iPhone using Tapatalk Pro
Re: Bug in execute_from_str
Posted: Wed Sep 12, 2018 2:16 pm
by BramPeeters
Because mp_parse 'frees' it. So 'collected' is the wrong word, it is explicitly freed by the code, not by the GC.
Code: Select all
mp_parse_tree_t mp_parse(mp_lexer_t *lex, mp_parse_input_kind_t input_kind)
{
....
mp_lexer_free(lex); <======
return parser.tree;
}
Re: Bug in execute_from_str
Posted: Wed Sep 12, 2018 2:18 pm
by jickster
Look into that function. It doesn’t free lex itself.
Sent from my iPhone using Tapatalk Pro
Re: Bug in execute_from_str
Posted: Wed Sep 12, 2018 2:22 pm
by BramPeeters
In my codebase it sure does ...
Code: Select all
void mp_lexer_free(mp_lexer_t *lex) {
if (lex) {
lex->reader.close(lex->reader.data);
vstr_clear(&lex->vstr);
m_del(uint16_t, lex->indent_level, lex->alloc_indent_level);
m_del_obj(mp_lexer_t, lex); <=============
}
}
#define m_del_obj(type, ptr) (m_del(type, ptr, 1))
#define m_del(type, ptr, num) ((void)(num), m_free(ptr))
void m_free(void *ptr) {
free(ptr);
...
}
Re: Bug in execute_from_str
Posted: Wed Sep 12, 2018 4:40 pm
by jickster
BramPeeters wrote: ↑Wed Sep 12, 2018 2:22 pm
In my codebase it sure does ...
Code: Select all
void mp_lexer_free(mp_lexer_t *lex) {
if (lex) {
lex->reader.close(lex->reader.data);
vstr_clear(&lex->vstr);
m_del(uint16_t, lex->indent_level, lex->alloc_indent_level);
m_del_obj(mp_lexer_t, lex); <=============
}
}
#define m_del_obj(type, ptr) (m_del(type, ptr, 1))
#define m_del(type, ptr, num) ((void)(num), m_free(ptr))
void m_free(void *ptr) {
free(ptr);
...
}
Ok you're right but it's in the example API usage, not the API itself.
Re: Bug in execute_from_str
Posted: Wed Sep 12, 2018 6:03 pm
by dhylands
Re: Bug in execute_from_str
Posted: Wed Sep 12, 2018 6:24 pm
by BramPeeters
Looks way better than the direct 0 I replaced lex->source_name with ;p
Thanks