#3507 fcopy doesn't handle large sizes

[Olly Stephens]

If I do:

fcopy $in $out -size 3221176172

the channel copying code receives a negative size, with
disastrous consequences.

Seems like it will only except a 32-bit signed integer
and does no error handling.

With today's modern filesystems, definitely needs to be
capable of handling larger numbers.

I did a quick visual inspection of the 8.5 release and
it looks the same.

Olly

[Don Porter]

Logged In: YES
user_id=80530

The internal routines that make
up the guts of [fcopy] pass around
"int" arguments to track the number
of bytes left to copy. All of
them would need to change to correct
this. So long as they are all internal,
that should be possible.

[Carlos Tasada]

Logged In: YES
user_id=340696
Originator: NO

Any chance to have it fixed in 8.5?

[Alexandre Ferrieux]

Logged In: YES
user_id=496139
Originator: NO

Half-fixed :-)
(Truly fixing would imply switching to at least wide ints or even worse... which may hit the performance of the dominant use of -size with a small value (who would do a fcopy of 4G without a progress indicator ?))

It is not true that there is no error handling: a value over 4G will elicit an "integer value too large to represent" error.
However, there was still the signed-int vs. unsigned-int issue: a value of 3G is silently accepted as negative, *and* negative values were not rejected.

The patch below (tclIOCmd.c) rejects negative values:

case FcopySize:
if (TclGetIntFromObj(interp, objv[i+1], &toRead) != TCL_OK) {
return TCL_ERROR;
}
+ if (toRead<0) {
+ Tcl_AppendResult(interp, "negative size forbidden",NULL);
+ return TCL_ERROR;
+ }
break;

[Alexandre Ferrieux]

Logged In: YES
user_id=496139
Originator: NO

Andreas, any comment ?

[Andreas Kupries]

Logged In: YES
user_id=75003
Originator: NO

Doing such a check is good. I do not like the returned message yet. It can happen for true negative sizes, and for overflow, and for the second case the message is not a good indicator what went wrong. I will play with that part of the fix to see if I can find a way to separate the 2 cases and return proper messages for each of them.

[Andreas Kupries]

Logged In: YES
user_id=75003
Originator: NO

Done, with modified check to separate overflow from true negatives.
Committed to 8.4, 8.5 branch, and head.

[Andreas Kupries]

Logged In: YES
user_id=75003
Originator: NO

Source diving revealed that '-size -1' is a special value which has meaning ('copy all'), which we should preserve.

After additional discussion with JeffH and Don Porter we will keep the check for 'toRead < 0', however it will not be an error, but simply reset toRead to the default -1. That means that from then on all negative values, and values out of the range warapping to negative will behave as -1, copy everything.

[Roy Keene]

Any chance this bug will actually get fixed ?