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

remove Windows output translation #218

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

slingamn
Copy link

@slingamn slingamn commented Feb 8, 2023

According to https://github.com/jwalton/go-supportscolor the technique here is valid on all supported versions of Windows.

Without this change, 256-color ANSI codes will panic. For example, 48; sets a background color from the 256-color palette and causes an out-of-bounds read here:

color |= ColorTableBg[c-40]

goroutine 14 [running]:
github.com/chzyer/readline.(*ANSIWriterCtx).ioloopEscSeq(0xc0000243c0?, 0xfe6142?, 0x6d, 0xc00001ef98)
        /redacted/ircdog/vendor/github.com/chzyer/readline/ansi_windows.go:164 +0x66f
github.com/chzyer/readline.(*ANSIWriterCtx).process(0xc00001ef90, 0x1b?)
        /redacted/ircdog/vendor/github.com/chzyer/readline/ansi_windows.go:99 +0xcf
github.com/chzyer/readline.(*ANSIWriter).Write(0xc00001efc0, {0xc000018640, 0xad, 0x140})
        /redacted/ircdog/vendor/github.com/chzyer/readline/ansi_windows.go:207 +0xdf
github.com/chzyer/readline.(*wrapWriter).Write.func1()
        /redacted/ircdog/vendor/github.com/chzyer/readline/operation.go:57 +0x44
github.com/chzyer/readline.(*RuneBuffer).Refresh(0xc000155cc0, 0xc00017bd40)
        /redacted/ircdog/vendor/github.com/chzyer/readline/runebuf.go:463 +0xd8
github.com/chzyer/readline.(*wrapWriter).Write(0xc00017bdb0, {0xc000018640?, 0xded668?, 0xc000021110?})
        /redacted/ircdog/vendor/github.com/chzyer/readline/operation.go:56 +0xa7
github.com/chzyer/readline.(*Instance).Write(0xc000021110?, {0xc000018640, 0xad, 0x140})
        /redacted/ircdog/vendor/github.com/chzyer/readline/readline.go:298 +0x85
fmt.Fprintln({0x1114660, 0xc000008378}, {0xc00017bef8, 0x1, 0x1})
        /usr/local/go/src/fmt/print.go:305 +0x75
main.connectExternal.func2()
        /redacted/ircdog/ircdog.go:303 +0x305
created by main.connectExternal
        /redacted/ircdog/ircdog.go:284 +0x2d9

Thanks very much for your time.

@wader
Copy link

wader commented Feb 8, 2023

Nice cleanup 👍 by "all supported versions of windows" you mean all version of windows supported by microsoft or golang?

@slingamn
Copy link
Author

slingamn commented Feb 8, 2023

By Microsoft, sorry. Go still supports Windows 7: golang/go#57003

@wader
Copy link

wader commented Feb 8, 2023

I see, thanks! some users of https://github.com/wader/fq seem to have some ANSI related issues on windows, for example underline with colored text somehow ends up changing the background color. I don't know much about windows or windows terminal so i'm bit lost :)

@wader
Copy link

wader commented Feb 8, 2023

btw fq uses this readline fork https://github.com/wader/readline that has a bunch of fixes, maybe interesting

This is now supported natively via ENABLE_VIRTUAL_TERMINAL_INPUT.
@wader
Copy link

wader commented Mar 6, 2023

Did some testing with fq with these changes, seems to work fine. Do you know if ANSI colors are natively support in the "old" cmd.exe also? i tested using virtualbox and image from https://developer.microsoft.com/en-us/windows/downloads/virtual-machines/

@slingamn
Copy link
Author

slingamn commented Mar 6, 2023

Yep, I tested with Windows 10 22H2 on metal in both PowerShell and cmd.exe. Both aspects of this patch (ANSI output rendering and input processing) are fully functional in both contexts.

FWIW I'm still working on contacting chzyer and putting together a fork.

@wader
Copy link

wader commented Mar 7, 2023

@slingamn Great 👍 let us know how it goes

@NCLnclNCL
Copy link

By Microsoft, sorry. Go still supports Windows 7: golang/go#57003

No, it end support

@slingamn
Copy link
Author

@NCLnclNCL technically it is still supported as long as Go 1.20 is supported, i.e. until Go 1.22 is released :-)

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.

None yet

3 participants