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

Byte overflow problem in SIXEL decoder #593

Closed
saitoha opened this issue Sep 22, 2016 · 11 comments
Closed

Byte overflow problem in SIXEL decoder #593

saitoha opened this issue Sep 22, 2016 · 11 comments

Comments

@saitoha
Copy link
Contributor

saitoha commented Sep 22, 2016

Forwarded from mattn/go-sixel#5.

The color register #255 is treated as 257th color in mintty.

st->color_index = 1 + st->params[0]; /* offset 1(background color) added */

It will cause overflow in

image->data[pos] = st->color_index;
.

saitoha added a commit to saitoha/mintty-sixel that referenced this issue Sep 23, 2016
@saitoha
Copy link
Contributor Author

saitoha commented Sep 23, 2016

test patch:
saitoha@7b6bf81

@mintty
Copy link
Owner

mintty commented Sep 23, 2016

I wonder why and how SIXEL colour registers interfere with mintty's text colour handling.
What if the colours are redefined (with escape sequences); would that spoil image display?
Maybe the two mechanisms should be decoupled?

@saitoha
Copy link
Contributor Author

saitoha commented Sep 25, 2016

I think if we need to emulate old hardware faithfully, text color registers have to be shared with that of SIXEL.
Actually, some old hardware users use -m(mapfile/fixed palette) option to integrate graphics on their 4bpp/8bpp framebuffer terminal.
But we can also implement separated color registers for each images. This strategy is convenient for users in modern GUI environment. Current mintty choose it.

@mintty
Copy link
Owner

mintty commented Sep 25, 2016

So if there is currently no dependence of SIXEL colors on text 256 colors (if I understood you correctly, haven't had time to test it yet, sorry), why was there this surprising interference of the 257th color with mintty default color indications? Just trying to understand.

@mintty
Copy link
Owner

mintty commented Sep 26, 2016

I was confused by "257th" color because a few internal attribute values above 256 are used for foreground etc by mintty. Unrelated, so forget my previous comments here.

About the patch: Do we really need all the type definitions, int8, int16, int32, uint8... I'm asking because that doesn't compile with an older gcc version.

saitoha added a commit to saitoha/mintty-sixel that referenced this issue Sep 27, 2016
…tty#593)

uint8_t -> uchar
uint32_t -> uint
int32_t for palette -> colour
@saitoha
Copy link
Contributor Author

saitoha commented Sep 27, 2016

OK I use definitions of src/std.h and src/config.h in a664ed8.
Memory leak issue reported by saitoha/libsixel#50 (comment) is fixed with e92d9a5.

@mintty
Copy link
Owner

mintty commented Sep 27, 2016

Thanks a lot.
See also saitoha/libsixel#50 (comment)

@saitoha
Copy link
Contributor Author

saitoha commented Sep 27, 2016

Thanks for merging!

@saitoha saitoha closed this as completed Sep 27, 2016
@mintty
Copy link
Owner

mintty commented Oct 7, 2016

Released 2.6.2.

@mintty
Copy link
Owner

mintty commented Mar 14, 2018

I'm afraid I have to revert the latest patch from this issue, in order to resolve #740.

mintty added a commit that referenced this issue Jul 13, 2019
revert reversion of Sixel colour registers patch (#593);
fixed memory holes in error cases
@mintty
Copy link
Owner

mintty commented Jul 13, 2019

Released 3.0.2.

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

2 participants