Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Hi Utkarsh, On Fri, Feb 19, 2021 at 10:44:08PM +0530, Utkarsh Gupta wrote: > Hi Axel, Salvatore, > > On Fri, Feb 19, 2021 at 2:44 PM Axel Beckert wrote: > > No issue popped up so far during production use on Stretch and Buster. > > I'd say, we can publish these in good conscience. > > Perfect, thanks for all your work on this! \o/ > I've uploaded to stretch-security (& pushed the commit and tag as well). > > Salvatore, do you want me to push to buster-security as well? No need, we will let finishing the upload by Axel himself. Moritz will then take care of releasing the DSA. Regards, Salvatore
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Hi Axel, Salvatore, On Fri, Feb 19, 2021 at 2:44 PM Axel Beckert wrote: > No issue popped up so far during production use on Stretch and Buster. > I'd say, we can publish these in good conscience. Perfect, thanks for all your work on this! \o/ I've uploaded to stretch-security (& pushed the commit and tag as well). Salvatore, do you want me to push to buster-security as well? - u
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Hi Axel, Sorry for the late reply, I was a bit occupied with my school homework. On Wed, Feb 17, 2021 at 8:59 AM Axel Beckert wrote: > > So I created one with the latest dsc (4.2.1-3+deb8u1) and added 2 > > commits on top of it. > > Thanks for the effort, but this seems to have a separate git root > (why?!?) instead of being simply based on the tag debian/4.2.1-3 which > points to commit 80eaffbae4363a79987e0e9fc07d1df96e0abd83. It's a different root because I took the latest .dsc and created a branch from there. More on this below. > I fixed it by cherry-picking and applying your relevant commits — with > proper commit messages _NOT_ including the whole (!) debian/changelog > in the commit message but just the relevant entry — on top of the old > jessie branch and force pushed it. > > (And unless "gbp dch" is used, I prefer to keep debian/changelog in > sync with the packaging changes and not adding debian/changelog > entries in a separate commit. But I didn't merge those two commits of > yours as I wasn't sure about the reasons for that and it's a > style/workflow thing and might subject to taste.) Indeed, like many others, I like to keep changelog entries separate. And the reasoning behind this is it actually helps when I have to backport stuff, so merging branches becomes easier with the only conflict in the d/ch file and I can just fix it because it's all tied together in a single commit. But that said, of course, everybody has a different way and I respect that, so thanks for fixing it! :) > > By importing the dsc and creating a branch out of it, the commit history, > > I am afraid, is lost. > > I don't see why this is necessary nor how this could have happened at > all. The task would have been so simple.. Actually, it was intentional and my follow-up question (about you wanting to restore all that) was to know if you actually want all the history back or not. But you already did restore, so all good, thanks! > > Do you want to restore all that? > > I already fixed it (except for tags, but you didn't seem to have > pushed the one expected tag anyways), because I was annoyed even > before I read that far in your mail. Yes, I didn't push the tag because I never uploaded it to jessie. I just prepared the upload and shall only tag the commit once I *actually* upload. I am sure you'd *not* want (me) to tag releases w/o actually uploading? :) > https://salsa.debian.org/debian/screen/-/commits/jessie/ now has a > proper history again. Ack, thanks. But...(more below). > > I'll be happy to do that after checking out from master and then > > applying +deb8u1 changes and then pushing mine on top of it, but I > > don't see any value to it, really. > > Huh, "no value"?!? Sorry, but a proper versioning is _essential_. Maybe I wasn't clear because there seems to be a misunderstanding here. When I said "no value", I meant not for the versioning part but "no value" in restoring all the history for the jessie branch. Because you see, next time somebody does the upload for jessie (or for stretch some years later) might not really care about pushing to salsa. Exactly what happened for deb8u1 upload. The general workflow is to get the latest dsc from the pool, do changes on top of that, and upload. And repeat. And this is because we don't expect all maintainers to actually keep a jessie branch alive and stuff. And some maintainers really, really don't like to be contacted for such trivial things. So we don't really disturb them from our end unless needed. Hopefully, I gave you the bigger picture and the real reasoning behind my "no value" phrase. Please let me know if it's still not clear or anything. I'll be happy to expand more! > Regards, Axel (not amused) ... :( - u
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Hi Utkarsh, Utkarsh Gupta wrote: > On Tue, Feb 16, 2021 at 11:12 PM Axel Beckert wrote: > > I'm running these patches (as in git) now for about 1.5 days on > > Stretch and Buster in production. I'd say if I don't find any > > regression until Wednesday evening (i.e. in 1 day), feel free to > > finalise the packages as needed (the're currently all sporting > > "UNRELEASED" in the debian/changelog) and release them. […] > Sure thing, once you give a go-ahead, I'll edit the changelog entry > and shall push that to the salsa repository. No issue popped up so far during production use on Stretch and Buster. I'd say, we can publish these in good conscience. Regards, Axel -- ,''`. | Axel Beckert , https://people.debian.org/~abe/ : :' : | Debian Developer, ftp.ch.debian.org Admin `. `' | 4096R: 2517 B724 C5F6 CA99 5329 6E61 2FF9 CD59 6126 16B5 `-| 1024D: F067 EA27 26B9 C3FC 1486 202E C09E 1D89 9593 0EDE signature.asc Description: PGP signature
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Dear Utkarsh, Utkarsh Gupta wrote: > Sure thing, once you give a go-ahead, I'll edit the changelog entry > and shall push that to the salsa repository. Ok, thanks! > > existing jessie branch in git. (It has no commit besides those being > > part of the master branch. Not sure why I or someone else has created > > it already.) > > There's no "jessie" branch on salsa, at least. Hmmm. $ git branch -av | fgrep jessie * jessie80eaffb Upload to unstable as 4.2.1-3 remotes/origin/jessie 80eaffb Upload to unstable as 4.2.1-3 Might be because the remote "origin" originally pointed to Alioth and I just edited .git/config manually instead of doing the "remote rename/add" dance since the Alioth remote won't come back. > So I created one with the latest dsc (4.2.1-3+deb8u1) and added 2 > commits on top of it. Thanks for the effort, but this seems to have a separate git root (why?!?) instead of being simply based on the tag debian/4.2.1-3 which points to commit 80eaffbae4363a79987e0e9fc07d1df96e0abd83. I fixed it by cherry-picking and applying your relevant commits — with proper commit messages _NOT_ including the whole (!) debian/changelog in the commit message but just the relevant entry — on top of the old jessie branch and force pushed it. (And unless "gbp dch" is used, I prefer to keep debian/changelog in sync with the packaging changes and not adding debian/changelog entries in a separate commit. But I didn't merge those two commits of yours as I wasn't sure about the reasons for that and it's a style/workflow thing and might subject to taste.) > By importing the dsc and creating a branch out of it, the commit history, > I am afraid, is lost. I don't see why this is necessary nor how this could have happened at all. The task would have been so simple: $ git checkout -b jessie debian/4.2.1-3 $ gbp import-dsc --ignore-branch ../screen_4.2.1-3.dsc $ git push -u origin jessie $ git push --tags > Do you want to restore all that? I already fixed it (except for tags, but you didn't seem to have pushed the one expected tag anyways), because I was annoyed even before I read that far in your mail. https://salsa.debian.org/debian/screen/-/commits/jessie/ now has a proper history again. > I'll be happy to do that after checking out from master and then > applying +deb8u1 changes and then pushing mine on top of it, but I > don't see any value to it, really. Huh, "no value"?!? Sorry, but a proper versioning is _essential_. Regards, Axel (not amused) -- ,''`. | Axel Beckert , https://people.debian.org/~abe/ : :' : | Debian Developer, ftp.ch.debian.org Admin `. `' | 4096R: 2517 B724 C5F6 CA99 5329 6E61 2FF9 CD59 6126 16B5 `-| 1024D: F067 EA27 26B9 C3FC 1486 202E C09E 1D89 9593 0EDE signature.asc Description: PGP signature
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Hi Axel, On Tue, Feb 16, 2021 at 11:12 PM Axel Beckert wrote: > I'm running these patches (as in git) now for about 1.5 days on > Stretch and Buster in production. I'd say if I don't find any > regression until Wednesday evening (i.e. in 1 day), feel free to > finalise the packages as needed (the're currently all sporting > "UNRELEASED" in the debian/changelog) and release them. > > Please push your changes and tags into the git repo. It's in > collab-maint^Wthe "debian" group, so you should have write access. Sure thing, once you give a go-ahead, I'll edit the changelog entry and shall push that to the salsa repository. > If you want to prepare a Jessie ELTS update, feel free to use the > existing jessie branch in git. (It has no commit besides those being > part of the master branch. Not sure why I or someone else has created > it already.) There's no "jessie" branch on salsa, at least. So I created one with the latest dsc (4.2.1-3+deb8u1) and added 2 commits on top of it. By importing the dsc and creating a branch out of it, the commit history, I am afraid, is lost. Do you want to restore all that? I'll be happy to do that after checking out from master and then applying +deb8u1 changes and then pushing mine on top of it, but I don't see any value to it, really. But with your maintainer hat on, if you want me to do that, I'll do so! :) The jessie branch atm looks like this: https://salsa.debian.org/debian/screen/-/tree/jessie Please let me know if you have any questions or suggestions or if you hoped to see something else than this? :) - u
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Hi Utkarsh, Utkarsh Gupta wrote: > > What so far was in git in the stretch and buster branches was > > incomplete and did FTBFS for multiple reasons. (Just pushed a bunch of > > fixes. It at least builds now on both releases.) > > > > And in Stretch the patch didn't even apply properly and needed manual > > massaging. So at least for Stretch (and Jessie) this needs additional > > testing. > > Oh sure! I'll hold off any work until uploads in other releases have > been settled down. I'm running these patches (as in git) now for about 1.5 days on Stretch and Buster in production. I'd say if I don't find any regression until Wednesday evening (i.e. in 1 day), feel free to finalise the packages as needed (the're currently all sporting "UNRELEASED" in the debian/changelog) and release them. Please push your changes and tags into the git repo. It's in collab-maint^Wthe "debian" group, so you should have write access. If you want to prepare a Jessie ELTS update, feel free to use the existing jessie branch in git. (It has no commit besides those being part of the master branch. Not sure why I or someone else has created it already.) I can also send you signed git commit hashes of my most recent commits in these branches if you prefer that to be on the safe side. Regards, Axel -- ,''`. | Axel Beckert , https://people.debian.org/~abe/ : :' : | Debian Developer, ftp.ch.debian.org Admin `. `' | 4096R: 2517 B724 C5F6 CA99 5329 6E61 2FF9 CD59 6126 16B5 `-| 1024D: F067 EA27 26B9 C3FC 1486 202E C09E 1D89 9593 0EDE
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Hi Axel, On Mon, Feb 15, 2021 at 12:13 PM Axel Beckert wrote: > Please slow down! > > What so far was in git in the stretch and buster branches was > incomplete and did FTBFS for multiple reasons. (Just pushed a bunch of > fixes. It at least builds now on both releases.) > > And in Stretch the patch didn't even apply properly and needed manual > massaging. So at least for Stretch (and Jessie) this needs additional > testing. Oh sure! I'll hold off any work until uploads in other releases have been settled down. - u
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Hi Utkarsh, Utkarsh Gupta wrote: > On Sun, Feb 14, 2021 at 9:03 PM Axel Beckert wrote: > > > Since it's been ~3 days, do you think now would be the time to prepare > > > and upload to buster and stretch? > > > > While I prepared the uploads in git, I haven't yet tested them on > > Stretch and Buster. Currently still running the patch from 4.8.0-4, > > mostly due to time contraints and an RC bug in aptitude. > > > > If you want some more coverage, I'd wait until Monday evening or > > Tuesday. I hope to get the equivalent of the 4.8.0-5 patch installed > > on Stretch and Buster later this evening. > > That sounds good! Meanwhile, I'll compare the debdiff (for stretch) > and try to prepare something similar for jessie as well! Please slow down! What so far was in git in the stretch and buster branches was incomplete and did FTBFS for multiple reasons. (Just pushed a bunch of fixes. It at least builds now on both releases.) And in Stretch the patch didn't even apply properly and needed manual massaging. So at least for Stretch (and Jessie) this needs additional testing. Regards, Axel -- ,''`. | Axel Beckert , https://people.debian.org/~abe/ : :' : | Debian Developer, ftp.ch.debian.org Admin `. `' | 4096R: 2517 B724 C5F6 CA99 5329 6E61 2FF9 CD59 6126 16B5 `-| 1024D: F067 EA27 26B9 C3FC 1486 202E C09E 1D89 9593 0EDE
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Hi, On Sun, Feb 14, 2021 at 9:03 PM Axel Beckert wrote: > > Since it's been ~3 days, do you think now would be the time to prepare > > and upload to buster and stretch? > > While I prepared the uploads in git, I haven't yet tested them on > Stretch and Buster. Currently still running the patch from 4.8.0-4, > mostly due to time contraints and an RC bug in aptitude. > > If you want some more coverage, I'd wait until Monday evening or > Tuesday. I hope to get the equivalent of the 4.8.0-5 patch installed > on Stretch and Buster later this evening. That sounds good! Meanwhile, I'll compare the debdiff (for stretch) and try to prepare something similar for jessie as well! - u
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Hi Utkarsh, Utkarsh Gupta wrote: > On Fri, Feb 12, 2021 at 11:07 AM Salvatore Bonaccorso > wrote: > > Thanks for all your coordinaton, investigation, work on this! > > Seconded! Thanks for all your awesome and super fast work, really! \o/ Your welcome! > > Sounds good. I propose to have the potential final patch as well first > > slightly exposed first in unstable for a couple of days (2-3)? to see > > if anybody reports any further problems and only then release an > > update in buster, stretch. > > The potential final patch was uploaded to unstable on 2021-02-11, I > believe? Depends on the time zone: Date: Thu, 11 Feb 2021 23:54:56 + Date: Fri, 12 Feb 2021 00:07:18 +0100 (From https://packages.qa.debian.org/s/screen/news/20210211T235456Z.html) > Did you hear any problems with that? Nope. > Any bug reports (none that I can see though!)? Nope. > Since it's been ~3 days, do you think now would be the time to prepare > and upload to buster and stretch? While I prepared the uploads in git, I haven't yet tested them on Stretch and Buster. Currently still running the patch from 4.8.0-4, mostly due to time contraints and an RC bug in aptitude. If you want some more coverage, I'd wait until Monday evening or Tuesday. I hope to get the equivalent of the 4.8.0-5 patch installed on Stretch and Buster later this evening. Regards, Axel -- ,''`. | Axel Beckert , https://people.debian.org/~abe/ : :' : | Debian Developer, ftp.ch.debian.org Admin `. `' | 4096R: 2517 B724 C5F6 CA99 5329 6E61 2FF9 CD59 6126 16B5 `-| 1024D: F067 EA27 26B9 C3FC 1486 202E C09E 1D89 9593 0EDE
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Hi Axel, On Fri, Feb 12, 2021 at 11:07 AM Salvatore Bonaccorso wrote: > Thanks for all your coordinaton, investigation, work on this! Seconded! Thanks for all your awesome and super fast work, really! \o/ > Sounds good. I propose to have the potential final patch as well first > slightly exposed first in unstable for a couple of days (2-3)? to see > if anybody reports any further problems and only then release an > update in buster, stretch. The potential final patch was uploaded to unstable on 2021-02-11, I believe? Did you hear any problems with that? Any bug reports (none that I can see though!)? Since it's been ~3 days, do you think now would be the time to prepare and upload to buster and stretch? - u
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
On Thu, Feb 11, 2021 at 11:39:09PM +0100, Axel Beckert wrote:
> The only thing it didn't fix so far is
> https://savannah.gnu.org/bugs/?31336 aka
> https://bugs.debian.org/600246 although I really had some hope that
> this might be fixed as a side-effect of your patch. :-)
Oh, that happens because the U+3099 combining char is also matched
in utf8_isdouble(). The code in ansi.c does not expect this. Thus
this is another fallout from that commit. ;)
A simple fix (other than removing the entries from the isdouble
table) is to move the curr->w_mbcs = 0xff setting after the
combining character handling:
diff --git a/ansi.c b/ansi.c
index 2a52edd..83b266d 100644
--- a/ansi.c
+++ b/ansi.c
@@ -692,10 +692,6 @@ register int len;
}
curr->w_rend.font = 0;
}
-# ifdef DW_CHARS
- if (curr->w_encoding == UTF8 && utf8_isdouble(c))
- curr->w_mbcs = 0xff;
-# endif
if (curr->w_encoding == UTF8 && c >= 0x0300 && utf8_iscomb(c))
{
int ox, oy;
@@ -730,6 +726,10 @@ register int len;
}
break;
}
+# ifdef DW_CHARS
+ if (curr->w_encoding == UTF8 && utf8_isdouble(c))
+ curr->w_mbcs = 0xff;
+# endif
font = curr->w_rend.font;
# endif
# ifdef DW_CHARS
Cheers,
Michael.
--
Michael Schroeder SUSE Software Solutions Germany GmbH
m...@suse.de GF: Felix Imendoerffer HRB 36809, AG Nuernberg
main(_){while(_=~getchar())putchar(~_-1/(~(_|32)/13*2-11)*13);}
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Hi Axel, [dropping upstream lists and other people, + team@s.d.o] On Thu, Feb 11, 2021 at 11:39:09PM +0100, Axel Beckert wrote: [...] > Salvatore, Utkarsh: Will also prepare and test at least patches in Git > for Buster and Stretch. (Hey, I don't want my mutt screen sessions to > be killed anymore when reading this thread. ;-) We should then > probably coordinate 1:1 who does the according stable-security and LTS > uploads. Thanks for all your coordinaton, investigation, work on this! Sounds good. I propose to have the potential final patch as well first slightly exposed first in unstable for a couple of days (2-3)? to see if anybody reports any further problems and only then release an update in buster, stretch. Regards, Salvatore
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Hi Michael, Michael Schroeder schrieb am Thu, Feb 11, 2021 at 04:54:46PM +: > I've dug a bit more into this, and the root cause of all the evil we > see seems to be that utf8_isdouble() does not return true for the > 0xdf00-0xdfff character range. This seems to be a mistake done > in commit b8fd0c833bbd910a525d270ebc8f7e87ee00cb0a in the year 2008! Kinda explains why the original submitter couldn't reproduce in 4.0.6 or so. Was though surprised to see this version still being around somewhere. > While adding the df00-dfff range to utf8_isdouble is already fixing > the segfault, I think we should still apply the changes I sent earlier > for extra safety. Seems as if that "ÿ " issue is gone now with your most recent patch. The crash is gone, too. And "e̤̒" is output properly, too. Yay, thanks! The only thing it didn't fix so far is https://savannah.gnu.org/bugs/?31336 aka https://bugs.debian.org/600246 although I really had some hope that this might be fixed as a side-effect of your patch. :-) (Also reenabling the patch in these bug reports still showed the regression discovered back then: https://bugs.debian.org/677512) So I'll switch the Debian package of screen to that third patch of yours. Thanks! Salvatore, Utkarsh: Will also prepare and test at least patches in Git for Buster and Stretch. (Hey, I don't want my mutt screen sessions to be killed anymore when reading this thread. ;-) We should then probably coordinate 1:1 who does the according stable-security and LTS uploads. Regards, Axel -- ,''`. | Axel Beckert , https://people.debian.org/~abe/ : :' : | Debian Developer, ftp.ch.debian.org Admin `. `' | 4096R: 2517 B724 C5F6 CA99 5329 6E61 2FF9 CD59 6126 16B5 `-| 1024D: F067 EA27 26B9 C3FC 1486 202E C09E 1D89 9593 0EDE signature.asc Description: PGP signature
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Hi again,
On Wed, Feb 10, 2021 at 11:01:58PM -, Tavis Ormandy wrote:
> On 2021-02-10, Axel Beckert wrote:
> > + else if (i < sizeof combchars / sizeof *combchars) {
>
> This doesn't seem right, I think it should be compared against the
> calloc param at the top of utf8_handle_comb(), but I don't really
> understand enough about unicode to know where that 0x802 comes from!
Ack, I seem to have missed one level of dereference at least, so the
calculated size is probably too small.
> --- encoding.c2020-02-05 12:09:38.0 -0800
> +++ encoding.c2021-02-10 15:00:05.0 -0800
> @@ -1357,6 +1357,9 @@
>int root, i, c1;
>int isdouble;
>
> + if (c > 0x801)
> +return;
> +
>c1 = mc->image | (mc->font << 8) | mc->fontx << 16;
>isdouble = c1 >= 0x1100 && utf8_isdouble(c1);
>if (!combchars)
While that fix indeed fixes the crash as did mine (probably by
accident :-), I in the meanwhile found rendering issue with both:
I currently assume that this code handles combining diacriticals, i.e.
unicode characters which modify the previous character. Since they can
be stacked and Tavis mentioned that he thinks the code only handles
UTF-8 characters with (max) two bytes, I toyed around with multiple
combining diacriticals in a row. (Yes, I'm aware that these are not
"more than two bytes" with regards to that limit mentioned above.)
I found that without any patch, screen rendered the combination of
e.g. "e̤̒"
* the ASCII letter "e", and
* U+0324 COMBINING DIAERESIS BELOW (size is two bytes in UTF-8)
* U+0312 COMBINING TURNED COMMA ABOVE (size is two bytes in UTF-8)
correctly. With both, Tavis as well as my patch, only U+0324 COMBINING
DIAERESIS BELOW is rendered and U+0312 COMBINING TURNED COMMA ABOVE is
not shown.
Then again, "l᪼" (ASCII "l" + U+1ABC COMBINING DOUBLE PARENTHESES
ABOVE which has a three bytes representation in UTF-8 and clearly
above 0x800) is rendered correctly without patch as well with your
patch and mine.
The same counts for "e톫" (ASCII "e" and U+1D1AB MUSICAL SYMBOL
COMBINING UP BOW which has a four bytes representation in UTF-8).
Will test Michael's second patch proposal later today. Looking very
forward to that. :-)
Kind regards, Axel
--
PGP: 2FF9CD59612616B5 /~\ Plain Text Ribbon Campaign, http://arc.pasp.de/
Mail: a...@deuxchevaux.org \ / Say No to HTML in E-Mail and Usenet
Mail+Jabber: a...@noone.org X
https://axel.beckert.ch/ / \ I love long mails: https://email.is-not-s.ms/
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Hi Tavis,
thanks for having a look into this!
Tavis Ormandy wrote:
> On 2021-02-10, Axel Beckert wrote:
> > + else if (i < sizeof combchars / sizeof *combchars) {
>
> This doesn't seem right, I think it should be compared against the
> calloc param at the top of utf8_handle_comb(),
Good point, thanks!
Your patch works fine on a first glance. No side effects like these
"ÿ " I saw with Michael's proposed patch. (But Michael's patch also
seems to care more about these 0x800 ff. values, so his and your patch
probably go into the right direction while my fix was rather generic
and perhaps too local.)
> but I don't really understand enough about unicode to know where
> that 0x802 comes from!
Will have a closer look into that direction tomorrow. But it indeed
sounds saner than my patch. And it is also much shorter and less
intrusive as it doesn't need to indent a whole block.
> I think for sure this code doesn't handle c > 0x801,
Which would mean it just can handle Unicode characters which are
represented by two bytes in UTF-8 representation. Because
that's what's special about characters around that value:
U+07FF NKO TAMAN SIGNis 0xDF 0xBF in UTF-8.
U+0800 SAMARITAN LETTER ALAF is 0xE0 0xA0 0x80 in UTF-8.
(according to gucharmap)
This also explains why we see c1 and c2 in the code (and what they
mean) but e.g. no c3. It suddenly all starts to make more sense, yes.
BTW, credit for the right hint goes to
https://stackoverflow.com/questions/47783583/generating-3-byte-0x800-to-0x-utf-8-encodings-in-java
> so maybe that's an acceptable fix?
I probably haven't looked around far enough for my patch, yes. Just
looked at where the backtrace of the crash pointed me (same if-clause
as you pointed out on oss-sec) and tried to guard that one very
generically.
P.S.: A disclaimer for those who aren't aware of it: I'm not a GNU
Screen developer. I'm just Debian's screen package maintainer trying
fix that package in time for Debian's next stable release. So I can't
break GNU Screen, just Debian's package of it. ;-)
Regards, Axel
--
,''`. | Axel Beckert , https://people.debian.org/~abe/
: :' : | Debian Developer, ftp.ch.debian.org Admin
`. `' | 4096R: 2517 B724 C5F6 CA99 5329 6E61 2FF9 CD59 6126 16B5
`-| 1024D: F067 EA27 26B9 C3FC 1486 202E C09E 1D89 9593 0EDE
signature.asc
Description: PGP signature
Bug#982435: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination
Hi again,
Axel Beckert wrote:
> On Wed, Feb 10, 2021 at 08:59:15AM -0500, Michael Schröder wrote:
> > diff --git a/src/encoding.c b/src/encoding.c
> > index 11c3c41..e1ea364 100644
> > --- a/src/encoding.c
> > +++ b/src/encoding.c
> > @@ -1164,7 +1164,9 @@ void utf8_handle_comb(unsigned int c, struct mchar
> > *mc)
> > if (c1 >= 0xd800 && c1 < 0xe000)
> > comb_tofront(root, c1 - 0xd800);
> > i = combchars[root]->prev;
> > - if (c1 == i + 0xd800) {
> > + if (i == (unsigned int)root)
> > + i = combchars[root ^ 1]->prev; /* steal from other
> > root */
> > + if (i == 0x800 || i == 0x801 || c1 == i + 0xd800) {
> > /* completely full, can't recycle */
> > mc->image = '?';
> > mc->font = 0;
>
> Thanks, but this seems to break the actual output.
>
> With that patch I now get "ÿ " after every wide character in the
> output. The beginning now looks like this for me (in the hope it will
> be passed properly through mail):
>
> 円ᆆᆿÿ 忿ᇎᆿÿ 忘ᆿᆿÿ 忿ᆾᆿÿ 応ᆿᆿÿ 忿ᆷᆿÿ 忑ᆿᆿÿ 忿ᇠᆿÿ 冺ᆿᆿÿ 忿ᇇᆿÿ 忟ᆿᆿÿ 忿ᆺᆿÿ 忳ᆿᆿÿ 忿ᅳᆿÿ 忣ᆿᆿÿ
> 忿ᇯᆿÿ 忇ᆿᆿÿ 忿ᇅᆿÿ
Axel Beckert wrote:
> So your bug report is already publicly visible at
> https://lists.gnu.org/archive/html/screen-devel/2021-02/msg0.html
> even though it is hidden on Savannah. (This is something those with
> admin access to the screen project on Savannah might want to review.)
Well, but my own line above seems to have crashed my screen session
through mutt at least once, but I can't reproduce this anymore — and I
wrote that line in Emacs in the very same screen session before hand
(by pasting that line).
Anyway, I seem to have been able to make up a patch (against 4.8.0 as
in Debian Unstable) which avoids the crash as well as the issue I
described in my previous mail which I cited above.
I though have no idea if the patch castrates any other functionality
or if it has unwanted side effects. Any review would be nice:
--- a/encoding.c
+++ b/encoding.c
@@ -1408,21 +1408,23 @@
}
/* FIXME: delete old char from all buffers */
}
- else if (!combchars[i])
-{
- combchars[i] = (struct combchar *)malloc(sizeof(struct combchar));
- if (!combchars[i])
- return;
- combchars[i]->prev = i;
- combchars[i]->next = i;
-}
- combchars[i]->c1 = c1;
- combchars[i]->c2 = c;
- mc->image = i & 0xff;
- mc->font = (i >> 8) + 0xd8;
- mc->fontx = 0;
- debug3("combinig char %x %x -> %x\n", c1, c, i + 0xd800);
- comb_tofront(root, i);
+ else if (i < sizeof combchars / sizeof *combchars) {
+if (!combchars[i])
+ {
+combchars[i] = (struct combchar *)malloc(sizeof(struct combchar));
+if (!combchars[i])
+ return;
+combchars[i]->prev = i;
+combchars[i]->next = i;
+ }
+combchars[i]->c1 = c1;
+combchars[i]->c2 = c;
+mc->image = i & 0xff;
+mc->font = (i >> 8) + 0xd8;
+mc->fontx = 0;
+debug3("combinig char %x %x -> %x\n", c1, c, i + 0xd800);
+comb_tofront(root, i);
+ }
}
#else /* !UTF8 */
The basic idea is to avoid an out of bounds array access at all by
first checking if "i" is bigger than the biggest index in the
combchars array.
I have no idea if the elements of the combchars array do have all the
same size. I just assume that all have the same size as the first
element.
At least that patch doesn't show that "ÿ " string after each wide
character. I though can imagine that it suppress maybe one or two
characters at the very end of the array.
I'm currently running that patch locally and will also trying to
create a patched version for the screen versions with which I run my
mail sessions to get a feeling for it in production.
P.S. to Utkarsh: That means I will prepare patches for Stretch and
Buster at least in Debian's git at
https://salsa.debian.org/debian/screen
Regards, Axel
--
,''`. | Axel Beckert , https://people.debian.org/~abe/
: :' : | Debian Developer, ftp.ch.debian.org Admin
`. `' | 4096R: 2517 B724 C5F6 CA99 5329 6E61 2FF9 CD59 6126 16B5
`-| 1024D: F067 EA27 26B9 C3FC 1486 202E C09E 1D89 9593 0EDE
signature.asc
Description: PGP signature
