Florian Fischer <florian.fischer@muhq.space> wrote:
> Change the file displayed in a window by writing the new file name> to the window's file member.> ---> v2: remove strdup call> > vis-lua.c | 3 +++> vis.c | 11 +++++++++++> vis.h | 2 ++> 3 files changed, 16 insertions(+)>
Hi Florian,
I'm not against this but I'm a little confused as to how this is
different from ":e". Is it because the window is not usable in the
`FILE_OPEN` event (and/or why can't you use ":e" in your plugin)?
- Randy
--
https://rnpnr.xyz/
GPG Fingerprint: B8F0 CF4C B6E9 415C 1B27 A8C4 C8D2 F782 86DF 2DC5
Hey Randy,
> I'm not against this but I'm a little confused as to how this is> different from ":e". Is it because the window is not usable in the> `FILE_OPEN` event (and/or why can't you use ":e" in your plugin)?
No problem. In your defense I did not explain the issue with the straight
forward attempt to implement the plugin in the first place.
Let me explain what happens if you use ":e" during an event caused by another
":e" command.
":e" (cmd_edit in vis-cmds.c) is implemented roughly like this:
1. Open the file to edit in a new window -> this triggers a FILE_OPEN and a WIN_OPEN event subsequently.
2. Swap the old and the new window <- Undefined behavior I guess.
3. Close the old window <- This is the problem.
If we execute another ":e" command during an event created in step 1 of the
previous command we cause a double free.
Since the old window of the first ":e" command will be closed and freed by
the second ":e" command during resolution of the first.
Flo
Florian Fischer <florian.fischer@muhq.space> wrote:
> Hey Randy,> > > I'm not against this but I'm a little confused as to how this is> > different from ":e". Is it because the window is not usable in the> > `FILE_OPEN` event (and/or why can't you use ":e" in your plugin)?> > No problem. In your defense I did not explain the issue with the straight> forward attempt to implement the plugin in the first place.> > Let me explain what happens if you use ":e" during an event caused by another> ":e" command.> > ":e" (cmd_edit in vis-cmds.c) is implemented roughly like this: > > 1. Open the file to edit in a new window -> this triggers a FILE_OPEN and a WIN_OPEN event subsequently.> 2. Swap the old and the new window <- Undefined behavior I guess.> 3. Close the old window <- This is the problem.> > If we execute another ":e" command during an event created in step 1 of the> previous command we cause a double free.> Since the old window of the first ":e" command will be closed and freed by> the second ":e" command during resolution of the first.> > Flo
I see, that makes a little more sense. I might add that to my list
of issues to look into. Maybe there is a simple way of handling
this case.
Comments on the patch follow:
> diff --git a/vis-lua.c b/vis-lua.c> index 4d783c5d..81b22111 100644> --- a/vis-lua.c> +++ b/vis-lua.c> @@ -2058,6 +2058,9 @@ static int window_newindex(lua_State *L) {> }> lua_pop(L, 1);> return ret;> + } else if (strcmp(key, "file") == 0 && lua_isstring(L, 3)) {> + const char* filename = lua_tostring(L, 3);> + vis_window_change_file(win, filename);
You should check the return value here. If it is successful return 0
from otherwise something like:
return luaL_argerror(L, 3, "failed to open");
> }> }> > diff --git a/vis.c b/vis.c> index 0c05558a..7f005057 100644> --- a/vis.c> +++ b/vis.c> @@ -505,6 +505,17 @@ bool vis_window_reload(Win *win) {> return true;> }> > +bool vis_window_change_file(Win *win, const char* filename) {> + File *file = file_new(win->vis, filename);> + if (!file)> + return false;> + file->refcount++;> + if (win->file)> + file_free(win->vis, win->file);> + win->file = file;> + view_reload(win->view, file->text);> +}
This should return true in the successful case.
- Randy
--
https://rnpnr.xyz/
GPG Fingerprint: B8F0 CF4C B6E9 415C 1B27 A8C4 C8D2 F782 86DF 2DC5
Heyhey!
On May 8, 2024 2:51:47 PM GMT+02:00, Randy Palamar <randy@rnpnr.xyz> wrote:
>Florian Fischer <florian.fischer@muhq.space> wrote:>> Hey Randy,>> >> > I'm not against this but I'm a little confused as to how this is>> > different from ":e". Is it because the window is not usable in the>> > `FILE_OPEN` event (and/or why can't you use ":e" in your plugin)?>> >> No problem. In your defense I did not explain the issue with the straight>> forward attempt to implement the plugin in the first place.>> >> Let me explain what happens if you use ":e" during an event caused by another>> ":e" command.>> >> ":e" (cmd_edit in vis-cmds.c) is implemented roughly like this: >> >> 1. Open the file to edit in a new window -> this triggers a FILE_OPEN and a WIN_OPEN event subsequently.>> 2. Swap the old and the new window <- Undefined behavior I guess.>> 3. Close the old window <- This is the problem.>> >> If we execute another ":e" command during an event created in step 1 of the>> previous command we cause a double free.>> Since the old window of the first ":e" command will be closed and freed by>> the second ":e" command during resolution of the first.>> >> Flo>>I see, that makes a little more sense. I might add that to my list>of issues to look into. Maybe there is a simple way of handling>this case.>>Comments on the patch follow:>>> diff --git a/vis-lua.c b/vis-lua.c>> index 4d783c5d..81b22111 100644>> --- a/vis-lua.c>> +++ b/vis-lua.c>> @@ -2058,6 +2058,9 @@ static int window_newindex(lua_State *L) {>> }>> lua_pop(L, 1);>> return ret;>> + } else if (strcmp(key, "file") == 0 && lua_isstring(L, 3)) {>> + const char* filename = lua_tostring(L, 3);>> + vis_window_change_file(win, filename);>>You should check the return value here. If it is successful return 0>from otherwise something like:>> return luaL_argerror(L, 3, "failed to open");>>> }>> }>> >> diff --git a/vis.c b/vis.c>> index 0c05558a..7f005057 100644>> --- a/vis.c>> +++ b/vis.c>> @@ -505,6 +505,17 @@ bool vis_window_reload(Win *win) {>> return true;>> }>> >> +bool vis_window_change_file(Win *win, const char* filename) {>> + File *file = file_new(win->vis, filename);>> + if (!file)>> + return false;>> + file->refcount++;>> + if (win->file)>> + file_free(win->vis, win->file);>> + win->file = file;>> + view_reload(win->view, file->text);>> +}>>This should return true in the successful case.
Ay, nice catch! I totally missed this
It feels like this should at least generate a compilation warning. I don't remember seeing one though
Cheers,
Silvan
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Florian Fischer <florian.fischer@muhq.space> wrote:
> Change the file displayed in a window by writing the new file name> to the window's file member.> > This is useful to change the displayed file during events.> Using the edit command during an event caused by a pervious edit> command causes a double free.> ---> v2 -> v3: add error handling> vis-lua.c | 5 +++++> vis.c | 12 ++++++++++++> vis.h | 2 ++> 3 files changed, 19 insertions(+)>
Thanks! Applied with an explicit return 0 instead of through
newindex_common(). This is inline with what we do elsewhere.
I will send a commit to enable warnings by default in the
configure script. I'm not really sure why they aren't enabled.
- Randy
--
https://rnpnr.xyz/
GPG Fingerprint: B8F0 CF4C B6E9 415C 1B27 A8C4 C8D2 F782 86DF 2DC5