Ticket UUID: | 2107634 | |||
Title: | extend maximum command length to maxint | |||
Type: | Patch | Version: | None | |
Submitter: | brlcad | Created on: | 2008-09-12 15:20:03 | |
Subsystem: | 25. Channel System | Assigned To: | dgp | |
Priority: | 9 Immediate | Severity: | ||
Status: | Closed | Last Modified: | 2009-10-20 05:11:09 | |
Resolution: | Accepted | Closed By: | dgp | |
Closed on: | 2009-10-19 22:00:51 | |||
Description: |
We ran into a problem in BRL-CAD with the maximum command length in Tcl and have a fix that extends the maximum command limit approximately two times where it presently fails. The provided patch pushes that limit up about 2x by checking for the overflow and clamping to maxint. If that maxint limit is exceeded, then it now returns -1 instead of crashing too. The current behavior without the patch is that 'length' ends up overflowing causing a crash inside Tcl_SetObjLength() where it tries to use length (which is negative maxint) as an array index. For those wondering just how we'd actually blow out that limit, our (.asc) ASCII file format in BRL-CAD is essentially a Tcl transcript. So to load a file, we just have Tcl parse the file. Being a CAD package, our files are geometry. One of the geometry types is a polygonal mesh... So when you have one mesh that is really big (which is really easy to do), you end up blowing out the command length. This fix should be credited to Bob Parker (BRL-CAD developer) if it is applied. The patch was made against 8.5.1 We fully admit that the patch isn't ideal, but it does resolve the problem from our standpoint without any negative consequences until the API can be improved. As you're undoubtedly well-aware, DoReadChars() and friends need to not use int everywhere for string/buffer length tracking, instead using size_t's or something similarly longer (and fixed) instead of int. Cheers! Sean | |||
User Comments: |
andreas_kupries added on 2009-10-20 05:11:09:
Thanks for the explanation. It makes sense. dgp added on 2009-10-20 05:00:51: allow_comments - 1 dgp added on 2009-10-20 05:00:48: accepted for 8.5.8 and 8.6b2. dgp added on 2009-10-20 04:46:56: The problem to avoid is when the "length" value passed to Tcl_(Attempt)SetObjLength no longer really represents what the caller desires because of integer overflow. The really trouble some case is when offset is already INT_MAX. Then the doubling rule led to an attempt to allocate a new string of length 2*offset + TCL_UTF_MAX + 1, which wraps around the unsigned integer range and allocates a string of length 2. Then later code tries to write long strings into a buffer of length 2 and segfaults. Shifting the location of the TCL_UTF_MAX offset in the calculation means that the calculation of length is limited to 2*offset, which can be no bigger that UINT_MAX -1, which when interpreted as a signed int is negative, and triggers the appropriate failure. The whole TCL_UTF_MAX shifting business is needed to satisfy the demands of the Tcl_ExternalToUtf() routine. The +1 that used to be there was apparently in place to make sure that space for a terminating NUL was allocated. Since the Tcl_*SetObjLength() routines already provide for this, it was removed. andreas_kupries added on 2009-10-20 03:54:56: dgp, the patch looks ok to me in general. Even if I do not fully understand the exact tweaks re dstNeeded and/or spaceLeft, with the TCL_UTF_MAX moving between them. dgp added on 2009-10-15 20:29:26: Second version of the patch adds similar fixes to FilterInputBytes(). dgp added on 2009-10-15 20:28:58: File Added - 346764: 2107634-2.patch dgp added on 2009-10-15 02:58:41: Attached a patch that better governs buffer growth so that ReadChars can keep reading chars up until the read that will overflow the limits of Tcl values. On that read attempt, there will be a panic. Please give the patch a try and see if it solves the BRL-CAD problems at least as well as the contributed patch. If there is some value in having ReadChars() return -1 rather than panic when the actual limits are reached, please explain in a followup comment. dgp added on 2009-10-15 02:55:06: File Added - 346653: 2107634.patch dgp added on 2009-10-15 02:42:47: In the submitted patch, when a read tries to go beyond the capacity of a Tcl_Value, ReadChars() returns -1. What does that do? dgp added on 2009-10-07 03:03:37: Related issue Bug 2418583 indicates that overflows with Tcl_ExternalToUtf() also need attention. dgp added on 2009-04-09 23:23:50: Your patch appears to be preventing the passing of a negative length value to Tcl_SetObjLength(). This means the new T_SOL behavior of panic on a negative length will be avoided by your patch. The next releases should not harm your patch's effectiveness. Leaving this open, since the failure of the caller(s) of T_SOL to take notice of and react to string length overflow remains a problem, and the development of a better patch to go into the core sources still needs doing. brlcad added on 2009-02-04 00:11:59: Thanks for the heads up. We'll have to evaluate/merge the change if we update our bundled version of Tcl/Tk too, so it's good to know. If/when we do before you examine the impact on your end, I'll update the patch. dgp added on 2009-02-03 23:22:17: Fixing another bug (2553906) we've recently added checks to Tcl_(Attempt)SetObjLength to reject negative length requests. I'll try to find time to examine what impact that might have on this patch. brlcad added on 2008-09-12 22:20:03: File Added - 293150: cmd_2x_bug.patch |