piaware crashes with itcl3 3.4.4

[llamaonaskateboard]

Debian testing has updated itcl3 from 3.4.3-3.1 to 3.4.4-1 and piaware now crashes on startup as follows:

$ piaware --debug
2022-12-29 07:04:29Z ****************************************************
2022-12-29 07:04:29Z piaware version 8.2 is running, process ID 286787
2022-12-29 07:04:29Z your system info is: Linux debian 6.0.0-4-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.0.8-1 (2022-11-11) x86_64 GNU/Linux
2022-12-29 07:04:29Z GPS: ::gpsdClient0: connect_completed
2022-12-29 07:04:29Z GPS: ::gpsdClient0: connection to gpsd at localhost/2947 failed: connection refused
2022-12-29 07:04:29Z GPS: ::gpsdClient0: closing gpsd socket, scheduling reconnect
2022-12-29 07:04:31Z Connecting to FlightAware adept server at piaware.flightaware.com/1200
2022-12-29 07:04:31Z Connection with adept server at piaware.flightaware.com/1200 established
2022-12-29 07:04:32Z TLS handshake with adept server at piaware.flightaware.com/1200 completed
2022-12-29 07:04:32Z TLS status: sha1_hash DA1AE49EC2E57B0B412AEEDBFA3F89F558F98F34 subject {CN=*.flightaware.com,OU=Operations,O=FlightAware LLC,L=Houston,ST=Texas,C=US} issuer {CN=FlightAware Intermediate,OU=Operations,O=FlightAware LLC,ST=TX,C=US} notBefore {Sep  1 00:00:00 2018 GMT} notAfter {Oct  2 00:23:26 2024 GMT} serial 204D certificate {-----BEGIN CERTIFICATE-----
[snip]
-----END CERTIFICATE-----
} sbits 256 cipher TLS_AES_256_GCM_SHA384 version TLSv1.3
2022-12-29 07:04:32Z require expected fields
2022-12-29 07:04:32Z make sure the notBefore time has passed
2022-12-29 07:04:32Z make sure the notAfter time has yet to occur
2022-12-29 07:04:32Z crack fields in the certificate and require some of them to be present
2022-12-29 07:04:32Z validate the common name
SpellFix: programming error
[1]    286785 IOT instruction

I added some extra logger lines to try and narrow down the issue and the issue appears to be with this line in validate_certificate_status in fa_adept_client.tcl:
if {![info exist subject(CN)] || ($subject(CN) != "*.flightaware.com" && $subject(CN) != "piaware.flightaware.com" && $subject(CN) != "adept.flightaware.com" && $subject(CN) != "eyes.flightaware.com")} {
I don't know much about tcl but changing info exist subject(CN) to info exists subject(CN) appears to fix it.

[busterb]

The error I get from a similar Debian system supports this being a spelling error too. Thanks for the quick workaround @llamaonaskateboard

2022-12-29 12:34:02Z piaware version 8.2 is running, process ID 2078733
2022-12-29 12:34:02Z your system info is: Linux cat 6.0.0-6-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.0.12-1 (2022-12-09) x86_64 GNU/Linux
2022-12-29 12:34:02Z GPS: ::gpsdClient0: connect_completed
2022-12-29 12:34:02Z GPS: ::gpsdClient0: connection to gpsd at localhost/2947 failed: connection refused
2022-12-29 12:34:02Z GPS: ::gpsdClient0: closing gpsd socket, scheduling reconnect
2022-12-29 12:34:04Z Connecting to FlightAware adept server at piaware.flightaware.com/1200
2022-12-29 12:34:04Z Connection with adept server at piaware.flightaware.com/1200 established
2022-12-29 12:34:04Z TLS handshake with adept server at piaware.flightaware.com/1200 completed
SpellFix: programming error
Aborted

[busterb]

I do wonder if there is a security implication to this. Since this is the code that validates that the TLS certificate is for a flightaware domain, would there be a way to craft a malicious CN that passes this check? I didn't see a way the incorrect evaluation of ![info exist subject(CN)] would affect the resulting logic, but I'm also not a TCL expert, so don't know if this is considered undefined behavior when evaluated.

[mutability]

info exist is equivalent to info exists (note "may be abbreviated" below) which is why it's worked fine in the past:

SYNOPSIS
       info option ?arg arg ...?
[...]
DESCRIPTION
       This command provides information about various internals of the Tcl interpreter.  The legal options (which may be abbreviated) are:
[...]  
       info exists varName
[...]

This seems like a build problem or some underlying bug in the main tcl package (none of this is itcl-specific); tcl shouldn't be aborting here. A syntax / naming error should produce a tcl-level error, not an abort. There's a potentially large body of code that might trigger this and the correct fix is to fix the tcl build.

(Here's the source of the abort, which is an internal assertion failure within the tcl interpreter in a code path that only executes if an abbreviated option name is used: https://github.com/tcltk/tcl/blob/72f03c2e7847dc62c23391a60c990d562e7ed369/generic/tclEnsemble.c#L2181)

There's no security implication beyond a possible DoS (but if you can intercept outbound connections then it's trivial to prevent piaware from connecting anyway)

[llamaonaskateboard]

itcl3 was the only package updated and manually downgrading it back to 3.4.3-3.1 also fixes the issue.

I managed to write a minimal repro using itcl and it certainly appears itcl3 3.4.4 and SpellFix don't work together:

#!/usr/bin/env tclsh

package require Itcl

itcl::class TestClass {
    method test {}
}

itcl::body TestClass::test {} {
    set foo "bar"
    if {[info exist foo]} {
        puts "foo exists"
    }
}

TestClass testClass
testClass test

$ ./test.tcl
SpellFix: programming error
[1]    315606 IOT instruction  ./test.tcl

@busterb Grabbing the 3.4.3-3.1 .deb from current stable and downgrading is probably a better fix than modifying piaware.

EDIT: Building itcl3 3.4.4 from source also has the same issue...

[mutability]

Aha, okay, I guess it is indeed an interaction with itcl then. itcl does interact quite deeply with the core interpreter so it wouldn't entirely surprise me.

I couldn't find good source code control / release notes for 3.4.4 but a brute-force diff shows that there are indeed ensemble-related changes where it looks like itcl is messing directly with the ensemble rewrite table (and, presumably, getting it wrong). (The ensemble rewrite table is what "spellfix" is messing with)

diff -urN itcl3.4.3/generic/itcl_ensemble.c itcl3.4.4/generic/itcl_ensemble.c
--- itcl3.4.3/generic/itcl_ensemble.c   2015-06-13 03:15:17.000000000 +0800
+++ itcl3.4.4/generic/itcl_ensemble.c   2019-07-01 06:47:44.000000000 +0800
@@ -1497,8 +1497,10 @@
         }
         if (ensPart != NULL) {
             cmdPtr = (Command*)ensPart->cmdPtr;
+           TclInitRewriteEnsemble(interp, 2, 1, objv);
             result = (*cmdPtr->objProc)(cmdPtr->objClientData,
                 interp, objc, objv);
+           TclResetRewriteEnsemble(interp, 1);
             return result;
         }
     }
@@ -1529,8 +1531,10 @@
 
     if (result == TCL_OK) {
         cmdPtr = (Command*)ensPart->cmdPtr;
+       TclInitRewriteEnsemble(interp, 2, 1, objv);
         result = (*cmdPtr->objProc)(cmdPtr->objClientData, interp,
             cmdlinec, cmdlinev);
+       TclResetRewriteEnsemble(interp, 1);
     }
     Tcl_DecrRefCount(cmdlinePtr);
 

diff -urN itcl3.4.3/generic/itclInt.h itcl3.4.4/generic/itclInt.h
--- itcl3.4.3/generic/itclInt.h 2015-06-13 03:15:17.000000000 +0800
+++ itcl3.4.4/generic/itclInt.h 2019-07-01 06:47:44.000000000 +0800
@@ -60,6 +60,18 @@
  */
 #if (TCL_MAJOR_VERSION == 8) && (TCL_MINOR_VERSION < 6)
 #define Tcl_GetErrorLine(interp) ((interp)->errorLine)
+#define TclInitRewriteEnsemble(interp, remove, insert, objv) { \
+    Interp *iPtr = (Interp *)(interp);                                 \
+    iPtr->ensembleRewrite.sourceObjs = (objv);                         \
+    iPtr->ensembleRewrite.numRemovedObjs = (remove);           \
+    iPtr->ensembleRewrite.numInsertedObjs = (insert);          \
+}
+#define TclResetRewriteEnsemble(interp, root) {                        \
+    Interp *iPtr = (Interp *)(interp);                                 \
+    iPtr->ensembleRewrite.sourceObjs = NULL;                   \
+    iPtr->ensembleRewrite.numRemovedObjs = 0;                  \
+    iPtr->ensembleRewrite.numInsertedObjs = 0;                 \
+}
 #endif
 
 #define ITCL_TCL_PRE_8_5 (TCL_MAJOR_VERSION == 8 && TCL_MINOR_VERSION < 5)
@@ -248,6 +260,10 @@
 MODULE_SCOPE Var * ItclVarHashCreateVar (TclVarHashTable * tablePtr, 
                                const char * key, int * newPtr);
 
+#undef TclInitRewriteEnsemble
+#undef TclResetRewriteEnsemble
+#define TclInitRewriteEnsemble(interp, remove, insert, objv)
+#define TclResetRewriteEnsemble(interp, root)
 #endif /* Version dependent defs and macros */
 
 

[llamaonaskateboard]

Fossil was a bit of a pain to navigate but I found the commit for those changes at https://core.tcl-lang.org/itcl/info/a036f93dad15f1d4

I've reported this issues upstream at https://core.tcl-lang.org/itcl/tktview?name=171b58b82b

[mutability]

Thanks for doing the work to report this upstream! I'll avoid the abbreviation in piaware for this specific case, but getting it fixed upstream is the right thing here - who knows how many other bits of code are affected..

[WhoAmI0501]

I am running PiAware for around 20-30 minutes now under bookworm and haven't seen some crashes like this.

Debian currently ships a version called 3.4.4-2, the changelog states the following:

itcl3 (3.4.4-2) unstable; urgency=medium

• Revert the change which ports Itcl 3 to Tcl 8.7 because it breaks
  abbreviations in namespaces like [info exist var]
  (closes: #1027932, #1027857).

-- Sergei Golovan [email protected] Fri, 06 Jan 2023 12:47:32 +0300

From what I understood from the issue which was mentioned by @llamaonaskateboard and also from my experiences, am I correct that this problem is solved now?