Skip to content

Reordered prompt done callback to avoid accessing out of bound history#3318

Merged
dmaluka merged 2 commits intomicro-editor:masterfrom
Neko-Box-Coder:ChainPromptDoneCallback
Jun 2, 2024
Merged

Reordered prompt done callback to avoid accessing out of bound history#3318
dmaluka merged 2 commits intomicro-editor:masterfrom
Neko-Box-Coder:ChainPromptDoneCallback

Conversation

@Neko-Box-Coder
Copy link
Contributor

@Neko-Box-Coder Neko-Box-Coder commented May 25, 2024 •
edited
Loading

According to https://pkg.go.dev/github.com/zyedidia/micro/v2@v2.0.13/internal/info#InfoBuf.Prompt
A lua script can provide a done callback when the prompt has finished (enter is pressed).

This works fine until the done callback itself wants to have another prompt, for example multiple questions to configure an action.

The behavior in main right now will crash with

Micro encountered an error: runtime.boundsError runtime error: index out of range [22] with length 22
runtime/panic.go:114 (0x43c59c)
github.com/zyedidia/micro/v2/internal/action/infopane.go:115 (0x8d658f)
github.com/zyedidia/micro/v2/cmd/micro/micro.go:478 (0x90e0a6)
github.com/zyedidia/micro/v2/cmd/micro/micro.go:397 (0x90d9af)
runtime/internal/atomic/types.go:194 (0x44111d)
runtime/asm_amd64.s:1695 (0x4754e1)

If you can reproduce this error, please report it at https://github.com/zyedidia/micro/issues

So if I have something like this

micro.InfoBar():Prompt("Question 1 > ", "", "", nil, OnQuestion1Done)

local Answer1 = ""

function OnQuestion1Done(resp, cancelled)
    if cancelled then return end
    Answer1 = resp
    micro.InfoBar():Prompt("Question 2 > ", "", "", nil, OnQuestion2Done)
end


function OnQuestion2Done(resp, cancelled)
    if cancelled then return end
    -- Do something with Answer1 and resp...
end

It won't work and crash inside the 2nd prompt.

This is due to the 2nd call in the prompt trying to access history while the first prompt has not finished modifying the history.
See
https://github.com/zyedidia/micro/blob/e9bd1b35f4ee62352a361fdec94684accd6f4874/internal/info/infobuffer.go#L97-L109

and https://github.com/zyedidia/micro/blob/e9bd1b35f4ee62352a361fdec94684accd6f4874/internal/info/infobuffer.go#L150-L154
where line 152 is calling the done callback and we are still modifying the history for the current prompt on line 153+

The proposed solution just needs to reorder the done callback to the end of the block where we have finished modifying the history so that the next prompt call can safely modify it.

@Neko-Box-Coder Neko-Box-Coder marked this pull request as draft May 25, 2024 17:32
@Neko-Box-Coder Neko-Box-Coder marked this pull request as ready for review May 25, 2024 17:42
@dmaluka
Copy link
Collaborator

dmaluka commented Jun 2, 2024 •
edited
Loading

where line 152 is calling the done callback and we are still modifying the history for the current prompt on line 153+

More precisely, what happens is:

  1. DonePrompt() calls your OnQuestion1Done callback, which calls Prompt() which adds a new (empty) item to the history:
i.History[ptype] = append(i.History[ptype], "")
  1. After that, DonePrompt() incorrectly writes the last entered text from the 1st prompt to this newly created history item for the 2nd prompt:
h := i.History[i.PromptType]
h[len(h)-1] = resp

since it doesn't realize that len(h) has changed and thus this is a wrong item, it should write to the last-but-one item instead.

  1. After that, DonePrompt() itself immediately detects that this new history item is a duplicate (since DonePrompt() itself has just assigned it with the same text as the previous history item) and incorrectly removes it from the list.

  2. So the history item for the 2nd prompt no longer exists. So when micro tries to access this history item when handling the 2nd prompt, it crashes.

Anyway, your fix looks correct.

dmaluka
dmaluka reviewed Jun 2, 2024
View reviewed changes
internal/info/infobuffer.go
break
}
}
i.PromptCallback(resp, false)
Copy link
Collaborator

@dmaluka dmaluka Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add empty line above this line?

Copy link
Contributor Author

@Neko-Box-Coder Neko-Box-Coder Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Collaborator

@dmaluka dmaluka Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove spaces from this empty line.

Copy link
Contributor Author

@Neko-Box-Coder Neko-Box-Coder Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Neko-Box-Coder Neko-Box-Coder force-pushed the ChainPromptDoneCallback branch from 434eec8 to 26f192c Compare June 2, 2024 17:56
@dmaluka dmaluka merged commit dd913df into micro-editor:master Jun 2, 2024
@dmaluka
Copy link
Collaborator

dmaluka commented Jun 2, 2024

Merged, thanks.

@Neko-Box-Coder Neko-Box-Coder deleted the ChainPromptDoneCallback branch June 4, 2024 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmaluka dmaluka dmaluka left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Neko-Box-Coder @dmaluka