cyd01 / KiTTY

:computer: KiTTY, a free telnet/ssh client for Windows
https://www.9bis.com/kitty
Other
1.55k stars 132 forks source link

OSC52 panic #495

Closed mgrant0 closed 10 months ago

mgrant0 commented 1 year ago

I enabled OSC52 support in tmux. I double-click on some text. I received the popup "allow OSC52 clipboard sync". The text is indeed copied to the clipboard. (marvellous!)

I do this one or two more times or so and then I receive this error (n.b. it may be that I am double clicking somewhere with little or no text):

The instruction at 0x000000007644A398 referenced memory at
0x0000000000000000.  The memory could not be read.

Click on OK to terminate the program
Click on CANCEL to debug the program

KiTTY also seems just to quit without this error if I double-click on something which results in nothing being copied (as in, you have nothing selected and hit ctrl-c). In this case, you wouldn't touch the local clipboard at all. Same as if you selected nothing in Windows and hit ctrl-C then ctrl-V, you get what was previously in the clipboard, not an empty clipboard.

Is there an option (if not, please can we have an option?) to bypass the "allow OSC52 clipboard sync" popup? Or perhaps, replace this with one of those fading messages that you don't need to confirm, for example "Kitty set clipboard".

mgrant0 commented 10 months ago

@unxed any chance of a fix for this one as well?

unxed commented 10 months ago

Pls remind after 14th thanks!

mgrant0 commented 10 months ago

@unxed it's after the 15th

mgrant0 commented 10 months ago

And the popup has its own issue now: https://github.com/cyd01/KiTTY/issues/497 Thanks

unxed commented 10 months ago

Merci! Will look into the code now.

unxed commented 10 months ago

@mgrant0 have problems reproducing. Can you please provide exact steps? Not very familiar with tmux, sorry!

unxed commented 10 months ago
  1. created ~/.tmux.conf with set -g set-clipboard off line.
  2. started tmux
  3. clicked (single, double, left button, right button, selection) on some text on screen (inside kitty's window)

nothing happens latest kitty from official website

mgrant0 commented 10 months ago

In KiTTY, I have my terminal set in Connection/Data to putty-256color

In my .tmux.conf I have this to enable OSC-52 support:

set -s set-clipboard external
set -as terminal-features ',tmux-256color:clipboard'
set -as terminal-overrides ',*-256color:Ms=\E]52;%p1%s;%p2%s\E\\\\'
set -g mouse on

then start tmux. Once in tmux, my $TERM looks like this:

% echo $TERM
tmux-256color

If I double click a word, I get the KiTTY OSC popup, answering yes, the word is in the windows paste buffer.

If I double click around enough, especially in an area where there's no text like below the prompt, KiTTY dies.

unxed commented 10 months ago
unxed@main-2018:~$ echo $TERM
screen

Guess I am doing something wrong. .tmux.conf should be in ~?

mgrant0 commented 10 months ago

Yes ~/.tmux.conf

The terminal lines in my example are not matching your actual $TERM which is why your not seeing the copy action. I'll have to try this tonight, the correct lines need to match terminal type 'screen'.

It took me some fiddling to get this to trigger the osc52 in kitty as you can see. If there's s better way, perhaps someone can chime in.

Nothing that tmux sends back to KiTTY should kill it. We'll get it to reproduce for you somehow.

mgrant0 commented 10 months ago

I have a simpler way to reproduce this. Just do this at a shell prompt. no tmux, just ssh in with KiTTY and enter this:

echo '^[]52;;^[\\'

where ^[ is the ESC character which you can type with ctrl-v ESC

I get this: image Click 'OK' Then, after a few seconds, KiTTY dies.

unxed commented 10 months ago

did this. nothing happens :(

изображение

mgrant0 commented 10 months ago

Did you see your OSC52 popup?

Which version are you running? I'm running the precompiled version from this page: https://www.fosshub.com/KiTTY.html KiTTY - 0.76.1.12 @ 23/05/2023-17:06:56(GMT). I have reproduced this on both Windows 10 and Windows 11 running natively (not in a VM).

I tried all 3, Classic, Portable, and No Compression. All 3 produce the effect for me. The Portable one produced this error before dying:

image

I am very surprised you can't reproduce this. How is it possible for my setup to be different or non-standard?

unxed commented 10 months ago

kitty_portable-0.76.1.12.exe Have no Windows, testing in Wine. But if a program crashes, shouldn't it happen on all platforms? OSC52 popup do not seen.

Can you pls try to replace the code in terminal.c starting with case 52: line to:

case 52:
{
    int status = MessageBox(NULL,
        "Allow OSC52 clipboard sync?", "PyTTY", MB_OKCANCEL);

    if (status == IDOK) {
        base64_decodestate _d_state;
        base64_init_decodestate(&_d_state);
        char* d_out = malloc(term->osc_strlen);
        if (!d_out) {
            // Failed to allocate memory
            break;
        }

        int d_count = base64_decode_block(
            term->osc_string+1, term->osc_strlen-1, d_out, &_d_state);

        uint32_t fmt;
        wchar_t* buffer = NULL; // Changed to wchar_t

        int cnt = MultiByteToWideChar(CP_UTF8, 0, (LPCCH)d_out, d_count, NULL, 0);
        if (cnt > 0) {
            buffer = calloc(cnt + 1, sizeof(wchar_t));
            if (!buffer) {
                // Failed to allocate memory
                free(d_out);
                break;
            }
            MultiByteToWideChar(CP_UTF8, 0, (LPCCH)d_out, d_count, buffer, cnt);
        }

        fmt = CF_UNICODETEXT;
        if (buffer) {
            int BufferSize = (wcslen(buffer) + 1) * sizeof(wchar_t);

            HGLOBAL hData = GlobalAlloc(GMEM_MOVEABLE, BufferSize);
            if (!hData) {
                // Failed to allocate global memory
                free(buffer);
                free(d_out);
                break;
            }

            void *GData = GlobalLock(hData);
            if (!GData) {
                // Failed to lock global memory
                GlobalFree(hData);
                free(buffer);
                free(d_out);
                break;
            }

            memcpy(GData, buffer, BufferSize);
            GlobalUnlock(hData);

            if (OpenClipboard(NULL)) {
                EmptyClipboard();
                if (!SetClipboardData(fmt, (HANDLE)hData)) {
                    GlobalFree(hData);
                }
                CloseClipboard();
            } else {
                GlobalFree(hData);
            }
        }

        free(buffer);
        free(d_out);
    }

    break;
}

Added some checks that may help.

unxed commented 10 months ago

Or you can try this precompiled binary:

KiTTY.zip

mgrant0 commented 10 months ago

I tried your precopiled version. This version does not crash, good! But if the length of the string you get from the remote is zero length, then, I think you should clear the Windows copy buffer to be consistent.

Also, can you remove the MesssageBox? Is this popup really necessary?

Thank you for your work on this!

mgrant0 commented 10 months ago

But if the length of the string you get from the remote is zero length, then, I think you should clear the Windows copy buffer to be consistent.

No, don't do this. You're current version does the correct thing here.

unxed commented 10 months ago

The code now checks for existence of environment variable OSC52ALLOWED. If it is set to any non-empty value, user will not be asked for confirmation any more.

case 52:
{
    char *env_check = getenv("OSC52ALLOWED");
    int status = IDOK; // Default to IDOK

    if (!env_check || strlen(env_check) == 0) {
        status = MessageBox(NULL,
            "Allow OSC52 clipboard sync?", "PyTTY", MB_OKCANCEL);
    }

    if (status == IDOK) {
        base64_decodestate _d_state;
        base64_init_decodestate(&_d_state);
        char* d_out = malloc(term->osc_strlen);

        if (!d_out) {
            // Failed to allocate memory
            break;
        }

        int d_count = base64_decode_block(
            term->osc_string+1, term->osc_strlen-1, d_out, &_d_state);

        uint32_t fmt;
        wchar_t* buffer = NULL; // Changed to wchar_t

        int cnt = MultiByteToWideChar(CP_UTF8, 0, (LPCCH)d_out, d_count, NULL, 0);
        if (cnt > 0) {
            buffer = calloc(cnt + 1, sizeof(wchar_t));

            if (!buffer) {
                // Failed to allocate memory
                free(d_out);
                break;
            }

            MultiByteToWideChar(CP_UTF8, 0, (LPCCH)d_out, d_count, buffer, cnt);
        }

        fmt = CF_UNICODETEXT;

        if (buffer) {
            int BufferSize = (wcslen(buffer) + 1) * sizeof(wchar_t);

            HGLOBAL hData = GlobalAlloc(GMEM_MOVEABLE, BufferSize);

            if (!hData) {
                // Failed to allocate global memory
                free(buffer);
                free(d_out);
                break;
            }

            void *GData = GlobalLock(hData);

            if (!GData) {
                // Failed to lock global memory
                GlobalFree(hData);
                free(buffer);
                free(d_out);
                break;
            }

            memcpy(GData, buffer, BufferSize);
            GlobalUnlock(hData);

            if (OpenClipboard(NULL)) {
                EmptyClipboard();

                if (!SetClipboardData(fmt, (HANDLE)hData)) {
                    GlobalFree(hData);
                }

                CloseClipboard();
            } else {
                GlobalFree(hData);
            }
        }

        free(buffer);
        free(d_out);
    }

    break;
}
unxed commented 10 months ago

KiTTY.zip

unxed commented 10 months ago

Just for your interest. Since I practically did not have time to study this bug deeply, I entrusted the solution to ChatGPT 4, and it solved it on the first try. No changes were needed in generated code.

Full conversation with ChatGPT (in Russian, but you can use Google Translate or in-browser translation): https://chat.openai.com/share/ee1d116b-1354-466a-8220-822ceeff0928

unxed commented 10 months ago

https://github.com/cyd01/KiTTY/pull/512

mgrant0 commented 10 months ago

OMG, you used GPTChat to fix this, amazing. I did read the code quite carefully and it looks perfectly reasonable.

Instead of depending on an environment variable, can you add an option to the KiTTY Configuration panel?

unxed commented 10 months ago

OMG, you used GPTChat to fix this, amazing. I did read the code quite carefully and it looks perfectly reasonable.

I thought that if I can't reproduce the bug, maybe I could feed the code to a robot and see if it can find the problem. And it actually found it!

can you add an option to the KiTTY Configuration panel

At least I can try :) Possibly we should discuss this with @cyd01 first.

mgrant0 commented 10 months ago

I think the configuration option would probably go under Windows->Behavior and be called "Warn before synchronizing clipboard" or if you prefer "Warn before OSC52 clipboard sync".

mgrant0 commented 10 months ago

By the way, I tried setting that environment variable like this and I still get the popup, so I guess this is the wrong place to set such an env variable

image

unxed commented 10 months ago

To set an environment variable in Windows so that KiTTY can recognize it, you can follow these steps:

Temporary (for current session):

  1. Open a Command Prompt (cmd.exe).
  2. Type set VAR_NAME=value and hit Enter.
    • Replace VAR_NAME with the name of the environment variable you want to set.
    • Replace value with the value you want to assign to the environment variable.

Permanent (across sessions):

  1. Right-click on the Computer icon on your desktop or in File Explorer, and select "Properties."
  2. Click on "Advanced system settings" on the left sidebar.
  3. Click the "Environment Variables" button under the "Advanced" tab.
  4. Under the "User variables" or "System variables" section, click "New."
    • User variables are specific to the logged-in user.
    • System variables are available to all users.
  5. Enter the variable name (VAR_NAME) and value (value).
  6. Click OK and restart any open command prompts or restart your system for the changes to take effect.

After setting the environment variable using one of the methods above, KiTTY should be able to access it.

(this is also ChatGPT-generated answer)

smprather commented 9 months ago

The env vars in the KiTTY configuration are set into the shell/program that is launched by the ssh command. I'll estimate that ssh -o "SetEnv ..." is used to set them. So the vars are not visible to the KiTTY Windows process.