gh-103656: Transfer f-string buffers to parser to avoid use-after-free #103896
Conversation
|
After off-line discussions with @pablogsal, we felt that the best way forward is to add metadata to tokens and use it to pass the fstring debug buffer from the tokenizer to the parser, store it in the arena and ensure that its lifetime is okay for us to use in later passes as well. cc @sunmy2019 whose review I can't request. |
Parser/tokenizer.c
Outdated
| @@ -432,6 +433,15 @@ update_fstring_expr(struct tok_state *tok, char cur) | |||
| case ':': | |||
| if (tok_mode->last_expr_end == -1) { | |||
| tok_mode->last_expr_end = strlen(tok->start); | |||
| PyObject *res = PyUnicode_DecodeUTF8( | |||
There was a problem hiding this comment.
Hummm, not sure if this is the place where we should be doing this. Isn't it better to set the metadata when we create the token itself? This way we don't need to pass down the token all the way to here. This function just updates the pointers and at the point when we have the token ready we set the metadata (because the pointers are ready).
There was a problem hiding this comment.
Let me push a commit to see what I mean
pablogsal
force-pushed
the
fstring-fix-last-expr-buffer-use-after-free
branch
2 times, most recently
from
02ab63e to
4cbd070
Compare
pablogsal
force-pushed
the
fstring-fix-last-expr-buffer-use-after-free
branch
from
4cbd070 to
51d7d83
Compare
… avoid use-after-free
|
This is leaking refs in its current state. My guess is that we are not cleaning the python objects for f-strings that don't get debug info. |
pablogsal
force-pushed
the
fstring-fix-last-expr-buffer-use-after-free
branch
from
5920b9f to
326059e
Compare
…rser to avoid use-after-free
|
Btw as an optimization we should only set the metadata if the f-string has a '=' |
…s to parser to avoid use-after-free
|
Found the refleaks, the only thing missing is to fix #103896 (comment) |
I can take that. |
I just did it 😉 |
Although I may have messed something in Windows? |
|
Thanks for the help on this @pablogsal. All changes look good. |
|
Looks great! |
closes: #103656