Tcl Source Code

A nice easy one...

On windows, if you redefine TCL_DSTRING_STATIC_SIZE to 
be small (4!) then it crashes in 
TclpGetEncodingNameFromEnvironment because it assumes 
the DString buffer is large enough. I've not seen 
anything stating a minimum size for this so assume 
this is 'allowed'.

Something like:

TclpGetEncodingNameFromEnvironment(bufPtr)
    Tcl_DString *bufPtr;
{
    Tcl_DStringInit(bufPtr);
+#define MIN_DSTRING_LEN   (3 + TCL_INTEGER_SPACE)
+#if TCL_DSTRING_STATIC_SIZE < MIN_DSTRING_LEN
+    Tcl_DStringSetLength(bufPtr, MIN_DSTRING_LEN);
+#endif
    wsprintfA(Tcl_DStringValue(bufPtr), "cp%d", GetACP
());
    return Tcl_DStringValue(bufPtr);
}

seems like the simplest fix, as not many people are 
going to hit this problem.

After hittng this problem i had a quick look for any 
other potentially similar problems and saw this in 
tclWinDde.c:

#line 367
Tcl_DStringSetLength(&dString,
 offset + TCL_INTEGER_SPACE);

I think this should be:

Tcl_DStringSetLength(&dString,
 Tcl_DStringLength(&dString) + offset + 
TCL_INTEGER_SPACE);


...but again it's only a problem if 
TCL_DSTRING_STATIC_SIZE is small.

Logged In: YES 
user_id=80530

committed

File Added - 173519: 1366195.patch

Logged In: YES 
user_id=80530


here's a patch that corrects
these problems and updates
some overlooked version number
bumps as well.

Logged In: YES 
user_id=80530

waiting for cvs...

Logged In: YES 
user_id=80530


looking again at the original
report, the code in
tclWinDde.c looks fine to me,
and looks like the right
way to fix the problem with
TclpGetEncodingNameFromEnvironment().
Patch coming soon.

Logged In: YES 
user_id=1386588

>The change that would help them a bit would be
>to patch Tcl's source code so that it did not
>assume a value of TCL_DSTRING_STATIC_SIZE, but
>just used whatever value the header provided.
>

ie. replace TDSS with literal "200"'s in the core?

>The patch submitted did the opposite, setting a
>limit on what value of TCL_DSTRING_STATIC_SIZE
>can get set.

You mean an extension comiled with TDSS==4 using
Tcl_GetEncodingNameFromEnvironment linked to a core
with a regular TDSS==200 wouldn't have it DString
lengthened, and would still crash.
Good point - i hadn't thought of that.
The fix would be to remove the #if-ery, and just always:

Tcl_DStringSetLength(bufPtr,(2*(3+TCL_INTEGER_SPACE)));

My patch was only intended to allow TDSS to be set low and
not cause a stack corruption. Allowing for varying
values of TDSS between extensions and the core is a whole
different ball game. My opinion is that it is merely
a documentation issue - caveat programmor.

Logged In: YES 
user_id=80530


No, all users of
TCL_DSTRING_STATIC_SIZE
ought to use the value
given to them in tcl.h.

Because we specify a
value publicly, we would
have to think carefully
before ever changing it, in
case some extensions use of
the value in an old header
conflcts with the new value
we would want in a new header.

So, making any change to
this value makes sense only
in very narrow circumstances,
like someone customizing Tcl
for their own specialized needs
(low-memory environment?) and
maintaining enough control to
not run into the "old extension"
problem.

The change that would help them
a bit would be to patch Tcl's
source code so that it did not
assume a value of 
TCL_DSTRING_STATIC_SIZE, but
just used whatever value the
header provided.

The patch submitted did the
opposite, setting a limit
on what value of 
TCL_DSTRING_STATIC_SIZE can
get set.

Logged In: YES 
user_id=1386588

dgp:
>The patch I'd rather have is changes to Tcl so it
>stops making assumptions about how large the value is.

The patch fixes the only case i can find of the core
assuming a certain size for TCL_DSTRING_STATIC_SIZE.

Or do you mean that, for example, a core compiled with
TDSS==300, should support an extension compiled with
TDSS==100? ie. DStringInit would need to pass in its
static buffer buffer size which would probably be needed
to be stored in the DString struct.
Sounds like it would break a lot of binary interfaces...
and not for a lot of gain. (So probably this isn't what
you meant ;-)

Logged In: YES 
user_id=80530


thanks for the patch.

Looking it over, I see it
would catch any problems
with a source hacker trying
to make TCL_DSTRING_STATIC_SIZE
smaller than Tcl assumes it is.

The patch I'd rather have is
changes to Tcl so it stops making
assumptions about how large
the value is.

File Added - 172901: tclWinInit.c-1.69-patch

Logged In: YES 
user_id=1386588

Here's the requested patch.

data_type - 360894

Logged In: YES 
user_id=80530


Submitter is correct that
since we have a symbolic
value, it would be better to
use it than to assume we know
what its value is.  Doing so
would allow source hackers to
try different values with less
worry about breaking stuff.

That said, supporting source
hackers is fairly low on our
To Do list; they can take
care of themselves for the
most part.

I could have left this as a
low priority bug report, but
I'm switching it to a feature
request.  There's nothing really
buggy here, since tweaking these
values isn't something we really
support.  

The change is something that
ought to be done, but unless
a contributed patch comes in,
the usual maintainers probably
will work on more pressing matters.

Logged In: YES 
user_id=79902

I'm not quite sure what to do about this given that the
submitter is changing a fundamental constant...