Tcl Source Code

jan.nijtmans added on 2019-02-05 22:12:24:

Good catch!

Minor point: I think a surrogate pair should result in a single char in the byte-array, but that's a minor addition to your commit.

Or ... let this situation result in an error (as in Don's pure-bytearray work, but that should wait for Tcl 9)

Thanks!

sebres added on 2019-02-05 22:36:31:

Well this may be never ending story, Jan.

This is called "bomb planting" between developers, and such things are really ticking bombs, because could cause endless cycle if one does not take care - like now in my fix - [42976af3f3035391]. There is sooooo many places in Tcl (and some Tcl-modules) where "uni-ch" is modified after call of UtfToUniChar (so in case of high surrogate it may never enter next step and do low surrogate, so will enter endless cycle).

Anyway I don't think that a public function should return 0, where it has previously never do that (it returned always a result >= 1), still worse it is dependend from value of output parameter now (so as result of previous execution).

Just think about some wrapping things like toupper/tolower handler's (they seem to be safe in Tcl, but what is about some modules doiong similar things and modifying "ch" in-between)?

So I'd not close this ticket, until we've verified all calls of TclUtfToUniChar/Tcl_UtfToUniChar in whole world of Tcl (and in Tcl self).

jan.nijtmans added on 2019-02-06 08:15:02:

Thanks. Good point!  I'm open for suggesions for improvement, but I don't think it's so dangerous as you think: why would extensions modify 'ch' in-between? It only happens when handling out-of-BMP codes, Tcl was (up to 8.6) not prepared to handle that at all. But let's be practical.

I already scanned all of the Tcl world I could find for dangerous Tcl_UtfToUniChar(), and didn't find one. You spotted the one exception I missed ...   ;-)    I bet for a bottle of Champagne that you won't find another one! (Hoping that that motivates you to do a more intensive search than that I did).

One more remark: Forget my remark about producing a single byte array entry: it would violate EIAS: It influences "string length" when the internal representation  changes to bytearray. We have to wait for TIP #497, or for Don's 'pure byte array' implementation (I don't think ther's a TIP yet for this, but there is a branch): One of those will fix the EIAS problem.

So, I think you can close this ticket now. Or you can take the challenge and prove me wrong: I don't think there are other places in Tcl anymore like this.

Thanks!

sebres added on 2019-02-06 10:08:37:

> why would extensions modify 'ch' in-between?

Well, I found already 2 pieces within cycles on my side, that doing this: the one by compare of 2 uni-strings (both have set ch to zero exactly as in fixed case), and another by fast compression of string (ch = uni-char from a dictionary, if it is within and shorter in bits).

The point is - in constructs like following, you could never be sure that "ch" has been never changed:

    while ( src < srcEnd ) {
	// ...here... (like `Tcl_UniChar ch = 0; /* to silence stupid compiler warning */`)
	src += TclUtfToUniChar(src, &ch);
	// ...or even here... (like `ch = SomeTransform(ch); *dst1++ = ch; *dst2++ = ch;`)
	// ...or even calc... (like `ch = OperationAsPartOfCheckSum(ch); ...; sum += ch;`)
    }

Just because previously the return value was always >= 1 (and also documented this way "The return value is the number of bytes read from src."). And it is very very long time so.

And simply think about what this could cause then (endless cycles, buffer overrun, etc).
Would not we pay too high penalty to retain stubs/EIAS/compatibility for rare case "char is out of BMP" (and glyphs are mostly unneeded by 99% of app's).

> I'm open for suggesions for improvement

And I have some of these:

• either a new function like Tcl_UtfToUniCharEx/TclUtfToUniCharEx that doing the new implementation (and let both previous Tcl_UtfToUniChar/TclUtfToUniChar untouched). Still better this new function could get new param len, that I always missed (one should previously be sure that src is NTS or long enough to avoid access violation).
• or an intended change of Stubs, so some new parameter like Tcl_UtfToUniChar(srcPtr, ch, bmpOnly) that controls new implementation as well as forses the developer change (and verify) his code.

Both ways retain the stability (avoids vulni/SF/BO) and give developers a chance for correct processing out of BMP chars (if it is needed at all) using this new function (stub).

Anyway are you agree that a theoretical incompatibility is many many times better as BO/segfault (or even potential vulnerability)?

sebres added on 2019-02-06 13:34:29:

Just to explain on the real example what I mean:

github/Abendstern/.../graphics/font.cxx#L101-L102

61   while (*str) {
62     if ... {
         // ... long block here ...
100    } else {
101      Tcl_UniChar ch;
102      str += Tcl_UtfToUniChar(str, &ch);
103      ch &= 0xFFFF;
104      if (!hasLoaded[ch]) const_cast<Font*>(this)->renderCharacter(ch);
         // ...
107    }
108  }

And I mean not the modification of "ch" (in line 103), which still looks safe but could be affected later by some high surrogates (larger as word)...

But as you can see, the the variable "ch" is local-scope variable here (let alone it is not initialized, but has the existence in this scope only), so if the compiler optimizes this to use the reserverd stack block (for some other variables/calls between 61-101), the value of this may be overwritten, so you will get BO and/or endless cycle.

And I found it immediatelly as I just started my search (as I had small pause only), so I would not exclude thousands of modules/apps could be affected similar ways.

sebres added on 2019-02-06 18:57:05:

Accidentally I remembered still one case (may be affected on my side). And it is - so here is the next case, where `ch` gets modified inside the cycle - (inplace) conversion from UTF-8 (via UTF-16LE) into UTF-16BE:

  #if TCL_UTF_MAX > 4
    __bswap_32(ch);
  #else
    __bswap_16(ch);
  #endif

So just by the way (it is already 3rd on "my" own projects;).

jan.nijtmans added on 2019-02-10 15:33:34:

Well, excellent job!   I have an idea how to prevent "planting a bomb", without sacrificing compatibility. Stay tuned!

jan.nijtmans added on 2019-02-18 20:48:39:

Proposed fix in [9f67c17d0173a8ee]

sebres added on 2019-02-18 21:35:50:

Thx, Jan!

+1 for the proposed logic;

I should still take a closer look considering all possible corner cases (but I assume it could be safe now).

Just to avoid a certain continuous misprediction, I would change the order everywhere, because (len == 1) is mostly the case (so valid for 99% e.g. for each single byte ascii). So possibly something like:

- if ((len == 1) && ((ch & 0xFC00) == 0xD800)) ...
+ if (((ch & 0xFC00) == 0xD800) && (len == 1)) ...

And would be this not enough as check?

- if ((len == 1) && ((ch & 0xFC00) == 0xD800)) ...
+ if ((ch >= 0xD800) && (len == 1)) ...

I think either it is 0xD800 + [0..0x3ff] or len > 1. Am I wrong?

sebres added on 2019-02-19 10:44:00:

BTW, what is about Tcl_UniCharToUtf, tclUtf.c:164? Should it not be fixed in a similar way?

jan.nijtmans added on 2019-02-25 21:24:31:

Implementation complete now, including all corner-cases I could think of. Please evaluate.

sebres added on 2019-02-25 22:32:06:

Thx, Jan! It looks promising.

Just BTW, seeing all this "new" changes:

- char buf[TCL_UTF_MAX + 1];
+ char buf[TCL_UTF_MAX + 1] = "";

Would not this introduce another small incompatibility now?
I mean does the pre-initialization of buffer become mandatory now (or what would happen if it is not initialized)?

Is it what you've called "spurious UTF-8 testcase failure"? :)

P.S. I'll evaluate all the corner cases of new implementation a bit later (too busy ATM).

jan.nijtmans added on 2019-02-27 22:00:48:

Pre-inititialization is not really necessary, it only has effect when handling lone lower surrogates, which is actually illegal (although there is a test-case for it). The original TIP already has this 'problem'. See: https://travis-ci.org/tcltk/tcl/jobs/499135282, that's the spurious failure I was talking about.

Merged to 8.7 and trunk.

Thanks!

jan.nijtmans added on 2019-02-27 22:07:13:

Hm..   that travis link is wrong, that's another failure. The spurious UTF-8 failure is gone, I cannot find it any more....  Anyway, it's very unlikely to happen in practice, less than 1 promile assuming initialization with random bits.

dgp added on 2019-03-01 17:04:24:

Sorry to report that these changes cause a new segfault in the Tk test
canvText-17.1

dgp added on 2019-03-01 17:10:31:

....and apparently many more segfaults and hangs in the test suite of
the Tk trunk.

dgp added on 2019-03-01 17:44:30:

Bisecting points to checkin 

https://core.tcl-lang.org/tcl/info/0e2621fc4be5f95a

as the origin of the problem.

dgp added on 2019-03-01 17:51:13:

Program received signal SIGSEGV, Segmentation fault.
0x00000000006864d3 in Tcl_UniCharToUtf (ch=8364, buf=0x7fffffffeffe "")
    at /home/dgp/fossil/tclbisect/generic/tclUtf.c:187
187         buf[2] = (char) ((ch | 0x80) & 0xBF);

#0  0x00000000006864d3 in Tcl_UniCharToUtf (ch=8364, buf=0x7fffffffeffe "")
    at /home/dgp/fossil/tclbisect/generic/tclUtf.c:187
#1  0x0000000000517299 in TkUniCharToUtf (ch=8364, buf=0x7fffffffeffe "")
    at /home/dgp/fossil/tk/unix/../generic/tkUtil.c:1261
#2  0x00000000004f989f in Tk_PostscriptFontName (tkfont=0xe41370, 
    dsPtr=0x7fffffffc700) at /home/dgp/fossil/tk/unix/../generic/tkFont.c:1722
#3  0x000000000045c2ea in Tk_PostscriptFont (interp=0x9a05b0, 
    psInfo=0x7fffffffca40, tkfont=0xe41370)
    at /home/dgp/fossil/tk/unix/../generic/tkCanvPs.c:824

dgp added on 2019-03-01 20:34:53:

A fix is now committed to the 8.7 and trunk branches.

Tcl_UtfToUniChar() was completely broken for single-byte UTF-8 sequences.
They generated Tcl_UniChar values pulled from uninitialized memory.

It is difficult to believe that a total breakage like this was not detected
by the Tcl test suite. Either we do not test the routine at all, or all of
our tests have been disabled in their effectiveness by internal replacement
of calls to the broken function by invocation of macros that function correctly.
This needs to be pursued and corrected.

I note also that it appears that the new stealth conversion to cp1252 encoding
raises the possibility of the encoded output being longer than the encoded 
input (single bytes replaced by double bytes). This will upset the
expectations of some callers that use these Tcl routines to do in-place
processing in a single buffer.

jan.nijtmans added on 2019-03-01 20:58:48:

Wow!  Thanks for locating the fix. I was already having a look, but you were faster than me!

Inline conversion is always dangerous, even the ToLower and ToUpper functions can result in longer output than the input. And illegal bytes, like a single 0xB9 would already be doubled before TIP #389. So, yes, the danger is there, but it already was....

Many Thanks!