From 47b1f4425678fcc051e9a75f59f6c4cd4f21b176 Mon Sep 17 00:00:00 2001 From: "jan.nijtmans" Date: Thu, 15 May 2014 14:54:45 +0000 Subject: Fix [3118489]: NUL in filenames. (On Windows, protect against invalid use of ':' in filenames as well) --- tests/cmdAH.test | 3 +++ unix/tclUnixFile.c | 6 ++++++ win/tclWinFile.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 70 insertions(+), 2 deletions(-) diff --git a/tests/cmdAH.test b/tests/cmdAH.test index dc61ac6..4ca90c6 100644 --- a/tests/cmdAH.test +++ b/tests/cmdAH.test @@ -104,6 +104,9 @@ test cmdAH-2.6.1 {Tcl_CdObjCmd} { list [catch {cd ""} msg] $msg } {1 {couldn't change working directory to "": no such file or directory}} +test cmdAH-2.6.3 {Tcl_CdObjCmd, bug #3118489} -returnCodes error -body { + cd .\0 +} -result "couldn't change working directory to \".\0\": no such file or directory" test cmdAH-2.7 {Tcl_ConcatObjCmd} { concat } {} diff --git a/unix/tclUnixFile.c b/unix/tclUnixFile.c index 29f1aba..c5f75a7 100644 --- a/unix/tclUnixFile.c +++ b/unix/tclUnixFile.c @@ -1111,6 +1111,12 @@ TclNativeCreateNativeRep( str = Tcl_GetStringFromObj(validPathPtr, &len); Tcl_UtfToExternalDString(NULL, str, len, &ds); len = Tcl_DStringLength(&ds) + sizeof(char); + if (strlen(Tcl_DStringValue(&ds)) < len - sizeof(char)) { + /* See bug [3118489]: NUL in filenames */ + Tcl_DecrRefCount(validPathPtr); + Tcl_DStringFree(&ds); + return NULL; + } Tcl_DecrRefCount(validPathPtr); nativePathPtr = ckalloc((unsigned) len); memcpy((void*)nativePathPtr, (void*)Tcl_DStringValue(&ds), (size_t) len); diff --git a/win/tclWinFile.c b/win/tclWinFile.c index ed0c40f..9bf63b1 100644 --- a/win/tclWinFile.c +++ b/win/tclWinFile.c @@ -1856,6 +1856,9 @@ TclpObjChdir( nativePath = (const TCHAR *) Tcl_FSGetNativePath(pathPtr); + if (!nativePath) { + return -1; + } result = (*tclWinProcs->setCurrentDirectoryProc)(nativePath); if (result == 0) { @@ -3200,13 +3203,69 @@ TclNativeCreateNativeRep( Tcl_WinUtfToTChar(str, len, &ds); if (tclWinProcs->useWide) { WCHAR *wp = (WCHAR *) Tcl_DStringValue(&ds); - for (; *wp; ++wp) { - if (*wp=='/') { + len = Tcl_DStringLength(&ds)>>1; + /* + ** If path starts with "//?/" or "\\?\" (extended path), translate + ** any slashes to backslashes but accept the '?' as being valid. + */ + if ((str[0]=='\\' || str[0]=='/') && (str[1]=='\\' || str[1]=='/') + && str[2]=='?' && (str[3]=='\\' || str[3]=='/')) { + wp[0] = wp[1] = wp[3] = '\\'; + str += 4; + wp += 4; + len -= 4; + } + /* + ** If there is a drive prefix, the ':' must be considered valid. + **/ + if (((str[0]>='A'&&str[0]<='Z') || (str[0]>='a'&&str[0]<='z')) + && str[1]==':') { + wp += 2; + len -= 2; + } + while (len-->0) { + if ((*wp < ' ') || wcschr(L"\"*:<>?|", *wp)) { + Tcl_DecrRefCount(validPathPtr); + Tcl_DStringFree(&ds); + return NULL; + } else if (*wp=='/') { *wp = '\\'; } + ++wp; } len = Tcl_DStringLength(&ds) + sizeof(WCHAR); } else { + char *p = Tcl_DStringValue(&ds); + len = Tcl_DStringLength(&ds); + /* + ** If path starts with "//?/" or "\\?\" (extended path), translate + ** any slashes to backslashes but accept the '?' as being valid. + */ + if ((str[0]=='\\' || str[0]=='/') && (str[1]=='\\' || str[1]=='/') + && str[2]=='?' && (str[3]=='\\' || str[3]=='/')) { + p[0] = p[1] = p[3] = '\\'; + str += 4; + p += 4; + len -= 4; + } + /* + ** If there is a drive prefix, the ':' must be considered valid. + **/ + if (((str[0]>='A'&&str[0]<='Z') || (str[0]>='a'&&str[0]<='z')) + && str[1]==':') { + p += 2; + len -= 2; + } + while (len-->0) { + if ((*p < ' ') || strchr("\"*:<>?|", *p)) { + Tcl_DecrRefCount(validPathPtr); + Tcl_DStringFree(&ds); + return NULL; + } else if (*p=='/') { + *p = '\\'; + } + ++p; + } len = Tcl_DStringLength(&ds) + sizeof(char); } Tcl_DecrRefCount(validPathPtr); -- cgit v0.12 From 267fb4eebd7345f715153cea17de47c2396d31f8 Mon Sep 17 00:00:00 2001 From: dgp Date: Thu, 15 May 2014 16:48:43 +0000 Subject: Portable test to demo bug otherwise seen only on Windows. --- tests/io.test | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/io.test b/tests/io.test index b99c4e9..b82f403 100644 --- a/tests/io.test +++ b/tests/io.test @@ -3960,6 +3960,26 @@ test io-32.11 {Tcl_Read from a pipe} {stdio openpipe} { } {{hello } {hello }} +test io-32.11.1 {Tcl_Read from a pipe} {stdio openpipe} { + file delete $path(pipe) + set f1 [open $path(pipe) w] + puts $f1 {chan configure stdout -translation crlf} + puts $f1 {puts [gets stdin]} + puts $f1 {puts [gets stdin]} + close $f1 + set f1 [open "|[list [interpreter] $path(pipe)]" r+] + puts $f1 hello + flush $f1 + set x "" + lappend x [read $f1 6] + puts $f1 hello + flush $f1 + lappend x [read $f1] + close $f1 + set x +} {{hello +} {hello +}} test io-32.12 {Tcl_Read, -nonewline} { file delete $path(test1) set f1 [open $path(test1) w] -- cgit v0.12 From 564d1813daa4ebd962bcafa76e4ce48659d16a26 Mon Sep 17 00:00:00 2001 From: dgp Date: Thu, 15 May 2014 18:38:22 +0000 Subject: Branch to demo bug introduced in the parent commit. --- tests/io.test | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/io.test b/tests/io.test index c325809..b28566b 100644 --- a/tests/io.test +++ b/tests/io.test @@ -3960,6 +3960,26 @@ test io-32.11 {Tcl_Read from a pipe} {stdio openpipe} { } {{hello } {hello }} +test io-32.11.1 {Tcl_Read from a pipe} {stdio openpipe} { + file delete $path(pipe) + set f1 [open $path(pipe) w] + puts $f1 {chan configure stdout -translation crlf} + puts $f1 {puts [gets stdin]} + puts $f1 {puts [gets stdin]} + close $f1 + set f1 [open "|[list [interpreter] $path(pipe)]" r+] + puts $f1 hello + flush $f1 + set x "" + lappend x [read $f1 6] + puts $f1 hello + flush $f1 + lappend x [read $f1] + close $f1 + set x +} {{hello +} {hello +}} test io-32.12 {Tcl_Read, -nonewline} { file delete $path(test1) set f1 [open $path(test1) w] -- cgit v0.12 From 8019dad5cd3e9bb29762e4d700f067eed9873084 Mon Sep 17 00:00:00 2001 From: dgp Date: Fri, 16 May 2014 12:56:01 +0000 Subject: More tests to demo the bug more directly. --- tests/io.test | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/io.test b/tests/io.test index b28566b..53b7105 100644 --- a/tests/io.test +++ b/tests/io.test @@ -1559,6 +1559,45 @@ test io-13.8 {TranslateInputEOL: auto mode: \r\n} { close $f set x } "abcd\ndef" +test io-13.8.1 {TranslateInputEOL: auto mode: \r\n} { + set f [open $path(test1) w] + fconfigure $f -translation lf + puts -nonewline $f "abcd\r\ndef" + close $f + set f [open $path(test1)] + fconfigure $f -translation auto + set x {} + lappend x [read $f 5] + lappend x [read $f] + close $f + set x +} [list "abcd\n" "def"] +test io-13.8.2 {TranslateInputEOL: auto mode: \r\n} { + set f [open $path(test1) w] + fconfigure $f -translation lf + puts -nonewline $f "abcd\r\ndef" + close $f + set f [open $path(test1)] + fconfigure $f -translation auto -buffersize 6 + set x {} + lappend x [read $f 5] + lappend x [read $f] + close $f + set x +} [list "abcd\n" "def"] +test io-13.8.3 {TranslateInputEOL: auto mode: \r\n} { + set f [open $path(test1) w] + fconfigure $f -translation lf + puts -nonewline $f "abcd\r\n\r\ndef" + close $f + set f [open $path(test1)] + fconfigure $f -translation auto -buffersize 7 + set x {} + lappend x [read $f 5] + lappend x [read $f] + close $f + set x +} [list "abcd\n" "\ndef"] test io-13.9 {TranslateInputEOL: auto mode: \r followed by not \n} { set f [open $path(test1) w] fconfigure $f -translation lf -- cgit v0.12 From 4830ed53a3b546ff699230e332c0a6d4fecf5a24 Mon Sep 17 00:00:00 2001 From: dgp Date: Fri, 16 May 2014 14:40:39 +0000 Subject: Bug fix - accept consumption of the trailing newline in crlf with no characters produced. Also delete false assertions. --- generic/tclIO.c | 46 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/generic/tclIO.c b/generic/tclIO.c index 139a05e..16fc685 100644 --- a/generic/tclIO.c +++ b/generic/tclIO.c @@ -5400,7 +5400,6 @@ ReadChars( * record \r or \n yet. */ - assert(dstRead + 1 == dstDecoded); assert(dst[dstRead] == '\r'); assert(statePtr->inputTranslation == TCL_TRANSLATE_CRLF); @@ -5421,7 +5420,6 @@ ReadChars( assert(dstWrote == 0); assert(dstRead == 0); - assert(dstDecoded == 1); /* * We decoded only the bare cr, and we cannot read a @@ -5476,6 +5474,13 @@ ReadChars( return 1; } + /* + * Revise the dstRead value so that the numChars calc + * below correctly computes zero characters read. + */ + + dstRead = numChars; + /* FALL THROUGH - get more data (dstWrote == 0) */ } @@ -5502,16 +5507,38 @@ ReadChars( } if (dstWrote == 0) { + ChannelBuffer *nextPtr; - /* - * We were not able to read any chars. Maybe there were - * not enough src bytes to decode into a char. Maybe - * a lone \r could not be translated (crlf mode). Need - * to combine any unused src bytes we have in the first - * buffer with subsequent bytes to try again. + /* We were not able to read any chars. */ + + assert (numChars == 0); + + /* + * There is one situation where this is the correct final + * result. If the src buffer contains only a single \n + * byte, and we are in TCL_TRANSLATE_AUTO mode, and + * when the translation pass was made the INPUT_SAW_CR + * flag was set on the channel. In that case, the + * correct behavior is to consume that \n and produce the + * empty string. + */ + + if (dst[0] == '\n') { + assert(statePtr->inputTranslation == TCL_TRANSLATE_AUTO); + assert(dstRead == 1); + + goto consume; + } + + /* Otherwise, reading zero characters indicates there's + * something incomplete at the end of the src buffer. + * Maybe there were not enough src bytes to decode into + * a char. Maybe a lone \r could not be translated (crlf + * mode). Need to combine any unused src bytes we have + * in the first buffer with subsequent bytes to try again. */ - ChannelBuffer *nextPtr = bufPtr->nextPtr; + nextPtr = bufPtr->nextPtr; if (nextPtr == NULL) { if (srcLen > 0) { @@ -5548,6 +5575,7 @@ ReadChars( statePtr->inputEncodingFlags &= ~TCL_ENCODING_START; + consume: bufPtr->nextRemoved += srcRead; if (dstWrote > srcRead + 1) { *factorPtr = dstWrote * UTF_EXPANSION_FACTOR / srcRead; -- cgit v0.12