Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jun 2, 2024

Conversation

Neko-Box-Coder
Copy link
Contributor

@Neko-Box-Coder Neko-Box-Coder commented May 25, 2024

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

func (i *InfoBuf) Prompt(prompt string, msg string, ptype string, eventcb func(string), donecb func(string, bool)) {
// If we get another prompt mid-prompt we cancel the one getting overwritten
if i.HasPrompt {
i.DonePrompt(true)
}
if _, ok := i.History[ptype]; !ok {
i.History[ptype] = []string{""}
} else {
i.History[ptype] = append(i.History[ptype], "")
}
i.HistoryNum = len(i.History[ptype]) - 1
i.HistorySearch = false

and

resp := string(i.LineBytes(0))
i.Replace(i.Start(), i.End(), "")
i.PromptCallback(resp, false)
h := i.History[i.PromptType]
h[len(h)-1] = resp

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

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.

@@ -160,6 +159,7 @@ func (i *InfoBuf) DonePrompt(canceled bool) {
break
}
}
i.PromptCallback(resp, false)
Copy link
Collaborator

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

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

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

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 zyedidia: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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants