Bug in Tcl_GetRange()?

[Georgios Petasis]

Hi all,

I am getting a crash in my application, and gdb reports this:

#0 0x0000003af4f4d258 in __memmove_ssse3_back () from /lib64/libc.so.6
#1 0x00007ffff7e74ec4 in SetUnicodeObj ()
from /opt/ActiveTcl-8.6/lib/libtcl8.6.so
#2 0x00007ffff7e75022 in Tcl_NewUnicodeObj ()
from /opt/ActiveTcl-8.6/lib/libtcl8.6.so
#3 0x00007ffff0d08772 in CDM_GetFirstAnnotatedTextRange(Tcl_Obj*,
Tcl_Obj*) ()
from /opt/Ellogon/CDM/Linux/x86_64/libCDM.so
#4 0x00007ffff0d1e542 in CDM_GetFirstAnnotatedTextRangeObjCmd(void*,
Tcl_Interp*, int, Tcl_Obj* const*) () from
/opt/Ellogon/CDM/Linux/x86_64/libCDM.so
#5 0x00007ffff7dc005e in TclNREvalObjv ()
from /opt/ActiveTcl-8.6/lib/libtcl8.6.so
#6 0x00007ffff7e2c299 in TEBCresume ()
from /opt/ActiveTcl-8.6/lib/libtcl8.6.so
#7 0x00007ffff7dbbd57 in TclNRRunCallbacks ()
from /opt/ActiveTcl-8.6/lib/libtcl8.6.so
#8 0x00007ffff7dc0a55 in TclEvalEx () from
/opt/ActiveTcl-8.6/lib/libtcl8.6.so
#9 0x00007ffff7e54e07 in Tcl_FSEvalFileEx ()
from /opt/ActiveTcl-8.6/lib/libtcl8.6.so
#10 0x00007ffff7e5a42d in Tcl_MainEx ()
from /opt/ActiveTcl-8.6/lib/libtcl8.6.so
#11 0x0000000000400a36 in main ()
(gdb)

The C code that generates this is:

Tcl_GetRange(Text, (int) start, (int) end - 1);

The problem is that the Text objects has a string with length 36361
characters (not bytes), and start is 139214 and end is 139240.
Both are way larger than the text (truncated due to a limit in mysql).

But should Tcl_GetRange() perform checks that the range is valid, or
should I make the check? The manual is not clear at this. From the fact
that it never returns an error, I thought checks were done.

(Looking at the C source, no checks are performed. The function just
returns "Tcl_NewUnicodeObj(stringPtr->unicode + first, last-first+1);")

George

[Alexandre Ferrieux]

On Jun 11, 8:32 pm, Georgios Petasis <peta...@iit.demokritos.gr>
wrote:

>
> But should Tcl_GetRange() perform checks that the range is valid, or
> should I make the check? The manual is not clear at this. From the fact
> that it never returns an error, I thought checks were done.

The comment in the source is clearer than the manual:

* The first and last indices are
* assumed to be in the appropriate range.

That function is the lower layer of [string range], whose
implementation does the necessary checks. If you need that extra layer
of service, use it ;)

-Alex

[Georgios Petasis]

I cannot use [string range], I am at the C level.
I have already implemented a wrapper around Tcl_GetRange that does the
necessary checks.

But the manual should say something about this. For example, it does not
say that the last parameter can be -1.

(Usually public API makes some checks. It is strange that Tcl_GetRange
does not, and this must be reported in the man page)

George

[Alexandre Ferrieux]

On Jun 11, 11:21 pm, Georgios Petasis <peta...@iit.demokritos.gr>

wrote:
> Στις 12/6/2013 00:10, ο/η Alexandre Ferrieux έγραψε:
>
>
>
>
>
>
>
>
>
> > On Jun 11, 8:32 pm, Georgios Petasis <peta...@iit.demokritos.gr>
> > wrote:
>
> >> But should Tcl_GetRange() perform checks that the range is valid, or
> >> should I make the check? The manual is not clear at this. From the fact
> >> that it never returns an error, I thought checks were done.
>
> > The comment in the source is clearer than the manual:
>
> >   *      The first and last indices are
> >   *      assumed to be in the appropriate range.
>
> > That function is the lower layer of [string range], whose
> > implementation does the necessary checks. If you need that extra layer
> > of service, use it ;)
>
> > -Alex
>
> I cannot use [string range], I am at the C level.
> I have already implemented a wrapper around Tcl_GetRange that does the
> necessary checks.

OK.

>
> But the manual should say something about this. For example, it does not
> say that the last parameter can be -1.

I don't know where you get this impression from; the code consistently
computes the length as (last-first+1), so passing -1 there should be
disastrous.

> (Usually public API makes some checks. It is strange that Tcl_GetRange
> does not, and this must be reported in the man page)

Since it's too late to change the semantics, and the added
functionality doesn't justify a new stub entry, we'll fix the doc.
Thanks for reporting.

-Alex

[Georgios Petasis]

Στις 12/6/2013 17:40, ο/η Alexandre Ferrieux έγραψε:
>> But the manual should say something about this. For example, it does not
>> say that the last parameter can be -1.
>
> I don't know where you get this impression from; the code consistently
> computes the length as (last-first+1), so passing -1 there should be
> disastrous.

Yes, but these computations are the last parameters to
Tcl_NewStringObj() & Tcl_NewUnicodeObj(). Passing -1 as last,
it will result in the last argument to both these functions to be
-first. Negative values for them means "until the end of the string", so
it works.
It will not work in case of ByteArrays though.

For the time being, my code pases -1 and uses strings. It is legacy
code, from many years back. If I am to change my code, what value should
I put instead of -1?

>
>> (Usually public API makes some checks. It is strange that Tcl_GetRange
>> does not, and this must be reported in the man page)
>
> Since it's too late to change the semantics, and the added
> functionality doesn't justify a new stub entry, we'll fix the doc.
> Thanks for reporting.

I also think that fixing the docs is the way to go...

George

[Alexandre Ferrieux]

On Jun 12, 8:41 pm, Georgios Petasis <peta...@iit.demokritos.gr>
wrote:

> Στις 12/6/2013 17:40, ο/η Alexandre Ferrieux έγραψε:
>
> >> But the manual should say something about this. For example, it does not
> >> say that the last parameter can be -1.
>
> > I don't know where you get this impression from; the code consistently
> > computes the length as (last-first+1), so passing -1 there should be
> > disastrous.
>
> Yes, but these computations are the last parameters to
> Tcl_NewStringObj() & Tcl_NewUnicodeObj(). Passing -1 as last,
> it will result in the last argument to both these functions to be
> -first. Negative values for them means "until the end of the string", so
> it works.

And what about when first==0 ?
Seriously, this razor-edge, nearly-working, undocumented nightmare of
semantics is appalling...
My fingers are already itching.

> It will not work in case of ByteArrays though.

Yes, and disaster has been avoided by an angstrom in this case:
consider

[format %.0s [binary format I 1]]

this triggers the following code-path:

- start with a pure byte-array of 4 bytes
- due to the precision parameter .0 call the following in
Tcl_AppendFormatToObj:

case 's':
if (gotPrecision) {
...
segment = Tcl_GetRange(segment, 0, precision - 1);

- so at this point we are calling Tcl_GetRange( PURE_BYTEARRAY , 0,
-1)
- due to special handling of byte arrays as you mentioned this does

Tcl_NewByteArrayObj(bytes+first, last-first+1);

- since first==0 and last==-1, this is

Tcl_NewByteArrayObj(bytes,0)

- which (phhhhhhhhhhhew !) is just the empty string

Surely you realize that a slightly changed codepath using a nonzero
"start" would have gone kaboom.

> For the time being, my code pases -1 and uses strings. It is legacy
> code, from many years back. If I am to change my code, what value should
> I put instead of -1?

I'd say Tcl_GetCharLength(obj)

>
> I also think that fixing the docs is the way to go...
>

OK, so:

(1) mentioning the lack of checks (that's consensual)

(2) doing something about last==-1

What do you thing about (2):

- keep it undocumented as it only half-works.
- document it and fix it to work also on byte arrays

?

-Alex

[Georgios Petasis]

Στις 13/6/2013 00:56, ο/η Alexandre Ferrieux έγραψε:
> On Jun 12, 8:41 pm, Georgios Petasis <peta...@iit.demokritos.gr>
> wrote:
>> Στις 12/6/2013 17:40, ο/η Alexandre Ferrieux έγραψε:
>>
>>>> But the manual should say something about this. For example, it does not
>>>> say that the last parameter can be -1.
>>
>>> I don't know where you get this impression from; the code consistently
>>> computes the length as (last-first+1), so passing -1 there should be
>>> disastrous.
>>
>> Yes, but these computations are the last parameters to
>> Tcl_NewStringObj() & Tcl_NewUnicodeObj(). Passing -1 as last,
>> it will result in the last argument to both these functions to be
>> -first. Negative values for them means "until the end of the string", so
>> it works.
>
> And what about when first==0 ?

Well, no need to call Tcl_GetRange() :-)

> Seriously, this razor-edge, nearly-working, undocumented nightmare of
> semantics is appalling...
> My fingers are already itching.
>
>> It will not work in case of ByteArrays though.
>
> Yes, and disaster has been avoided by an angstrom in this case:
> consider
>
> [format %.0s [binary format I 1]]
>
> this triggers the following code-path:
>
> - start with a pure byte-array of 4 bytes
> - due to the precision parameter .0 call the following in
> Tcl_AppendFormatToObj:
>
> case 's':
> if (gotPrecision) {
> ...
> segment = Tcl_GetRange(segment, 0, precision - 1);
>
> - so at this point we are calling Tcl_GetRange( PURE_BYTEARRAY , 0,
> -1)
> - due to special handling of byte arrays as you mentioned this does
>
> Tcl_NewByteArrayObj(bytes+first, last-first+1);
>
> - since first==0 and last==-1, this is
>
> Tcl_NewByteArrayObj(bytes,0)
>
> - which (phhhhhhhhhhhew !) is just the empty string
>
> Surely you realize that a slightly changed codepath using a nonzero
> "start" would have gone kaboom.

Of course I realise this, but I am sure I have written the code before
Tcl get byte array objects... :-) (around 1999?)

>
>> For the time being, my code pases -1 and uses strings. It is legacy
>> code, from many years back. If I am to change my code, what value should
>> I put instead of -1?
>
> I'd say Tcl_GetCharLength(obj)
>

This will give exactly the same effect I now get with -1?

In the code of "string range", I see "end" is:
/*Get the length in actual characters; Then reduce it by one because
'end' refers to the last character, not one past it. */
length = Tcl_GetCharLength(objv[1]) - 1;

So, here, we have Tcl_GetCharLength(objv[1]) - 1. What does it mean to
use Tcl_GetCharLength(obj)? (Does it also count the null byte at the end?)

The code in Tcl_GetCharLength() is quite similar to the one in
Tcl_GetRange() (minus the COMPAT define, which I don't know why it
exists in Tcl_GetCharLength()). So, in case the object has
stringPtr->hasUnicode == 0, then Tcl_GetCharLength() is the way to go.

Is case stringPtr->hasUnicode!=0, it uses UnicodeLength(). The code in
there counts characters, until a null is found.

Will this be the same value as Tcl_GetCharLength()?

So the question is, what should I use instead of -1?
a) Always Tcl_GetCharLength()?
b) Always Tcl_GetCharLength()-1?
c) Check whether it has a unicode representation, and use
Tcl_GetCharLength()/UnicodeLength()?
d) As above, but remove -1?

(Passing -1 was much much simpler :D)

>>
>> I also think that fixing the docs is the way to go...
>>
>
> OK, so:
>
> (1) mentioning the lack of checks (that's consensual)
>
> (2) doing something about last==-1
>
> What do you thing about (2):
>
> - keep it undocumented as it only half-works.
> - document it and fix it to work also on byte arrays

I would choose the second option, as passing "-1" as the length
parameter in string related functions is something "widespread" in the
tcl public api. (I see -1 as the "end" at the tcl level)

But I don't know if the lack of checks in Tcl_GetRange() are related to
speed optimisations (i.e. it is called too often by the care), and if
adding them will have an impact on execution speed. In this case, I
would prefer to leave it as it is, and add a warning in the manual: do
not pass -1 as end, use a)/b)/c)/d) from above.

Regards,

George

[Alexandre Ferrieux]

On Jun 14, 12:46 pm, Georgios Petasis <peta...@iit.demokritos.gr>
wrote:
>

> >> For the time being, my code pases -1 and uses strings. It is legacy
> >> code, from many years back. If I am to change my code, what value should
> >> I put instead of -1?
>
> > I'd say Tcl_GetCharLength(obj)
>
> This will give exactly the same effect I now get with -1?

Sorry I meant Tcl_GetCharLength(obj)-1 of course ;)

> So the question is, what should I use instead of -1?
> a) Always Tcl_GetCharLength()?
> b) Always Tcl_GetCharLength()-1?
> c) Check whether it has a unicode representation, and use
> Tcl_GetCharLength()/UnicodeLength()?
> d) As above, but remove -1?

Answer (b): indeed, Tcl_GetCharLength counts characters, not bytes,
and that is also the unit of Tcl_GetRange.

-Alex

[Georgios Petasis]

This -1 is because Tcl_GetCharLength() counts also the terminating
"null" character of the string contained in the tcl object?

George

[Alexandre Ferrieux]

On Jun 14, 2:10 pm, Georgios Petasis <peta...@iit.demokritos.gr>

No, it is because the length of result is defined as "last-first
+1" (because of the "inclusive" tradition of [lrange]'s last
parameter).

-Alex