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

OSC52 panic #495

Closed
mgrant0 opened this issue Jun 12, 2023 · 27 comments
Closed

OSC52 panic #495

mgrant0 opened this issue Jun 12, 2023 · 27 comments

Comments

@mgrant0
Copy link

mgrant0 commented Jun 12, 2023

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
Copy link
Author

mgrant0 commented Sep 10, 2023

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

@unxed
Copy link
Contributor

unxed commented Sep 10, 2023

Pls remind after 14th thanks!

@mgrant0
Copy link
Author

mgrant0 commented Sep 15, 2023

@unxed it's after the 15th

@mgrant0
Copy link
Author

mgrant0 commented Sep 15, 2023

And the popup has its own issue now: #497
Thanks

@unxed
Copy link
Contributor

unxed commented Sep 15, 2023

Merci! Will look into the code now.

@unxed
Copy link
Contributor

unxed commented Sep 15, 2023

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

@unxed
Copy link
Contributor

unxed commented Sep 15, 2023

  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
Copy link
Author

mgrant0 commented Sep 15, 2023

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
Copy link
Contributor

unxed commented Sep 15, 2023

unxed@main-2018:~$ echo $TERM
screen

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

@mgrant0
Copy link
Author

mgrant0 commented Sep 16, 2023

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
Copy link
Author

mgrant0 commented Sep 16, 2023

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
Copy link
Contributor

unxed commented Sep 16, 2023

did this. nothing happens :(

изображение

@mgrant0
Copy link
Author

mgrant0 commented Sep 16, 2023

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
Copy link
Contributor

unxed commented Sep 17, 2023

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
Copy link
Contributor

unxed commented Sep 17, 2023

Or you can try this precompiled binary:

KiTTY.zip

@mgrant0
Copy link
Author

mgrant0 commented Sep 17, 2023

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
Copy link
Author

mgrant0 commented Sep 17, 2023

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
Copy link
Contributor

unxed commented Sep 17, 2023

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
Copy link
Contributor

unxed commented Sep 17, 2023

KiTTY.zip

@unxed
Copy link
Contributor

unxed commented Sep 17, 2023

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
Copy link
Contributor

unxed commented Sep 17, 2023

#512

@mgrant0
Copy link
Author

mgrant0 commented Sep 17, 2023

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
Copy link
Contributor

unxed commented Sep 17, 2023

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
Copy link
Author

mgrant0 commented Sep 17, 2023

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
Copy link
Author

mgrant0 commented Sep 17, 2023

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
Copy link
Contributor

unxed commented Sep 17, 2023

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
Copy link

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.

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

No branches or pull requests

3 participants