From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adolf Belka To: development@lists.ipfire.org Subject: Re: [PATCH] squid-asnbl: Fix for bug#13023 - squid-asnbl-helper segfaulting and shutdown squid Date: Wed, 17 May 2023 14:37:30 +0200 Message-ID: In-Reply-To: <20230322182852.3449030-1-adolf.belka@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1439059577391498134==" List-Id: --===============1439059577391498134== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi All this patch is still waiting for a decision/review? Regards, Adolf. On 22/03/2023 19:28, Adolf Belka wrote: > - Patch provided by bug reporter. Here is the description of the problem fr= om the bug. > First I discovered that the helper only sometimes throwing the error an= d quits even > for the same values and queries. Also the timespan until the error happ= ens was quite > different for every restart of squid (minutes to hours). And it does n= ot depend on > the traffic on the proxy, even one connection could cause a crash while= ten or > hundrets won't. After a few days of testing different solutions and don= e a lot of > debugging, redesigning the function did not fully solve the problem. Su= ch standard > things like checking the result variable for NULL (or it's equivalent "= is None" in > python) before evaluating it's subfunction produces the exact same erro= r message. But > with that knowledge it more and more turns out that python3 sometimes '= detects' the > local return variable if it was a misused global. So for a full fix, th= e return > variable also has to be initialized that python3 won't detect it's usag= e as an > 'UnboundLocalError' to succesfully fix this bug. > - LFS file updated to run patch before copying helper into place. > - Update of rootfile not needed. > - Bug reporter has been requested to raise this issue at the git repo for s= quid-asnbl. >=20 > Fixes: Bug#13023 > Tested-by: Nicolas P=D3=A7hlmann > Signed-off-by: Adolf Belka > --- > lfs/squid-asnbl | 1 + > ...les_to_make_compatible_with_python_3.patch | 100 ++++++++++++++++++ > 2 files changed, 101 insertions(+) > create mode 100644 src/patches/squid/squid-asnbl-0.2.4_initialise_global_= variables_to_make_compatible_with_python_3.patch >=20 > diff --git a/lfs/squid-asnbl b/lfs/squid-asnbl > index 130b28460..b003d605b 100644 > --- a/lfs/squid-asnbl > +++ b/lfs/squid-asnbl > @@ -75,6 +75,7 @@ $(subst %,%_BLAKE2,$(objects)) : > $(TARGET) : $(patsubst %,$(DIR_DL)/%,$(objects)) > @$(PREBUILD) > @rm -rf $(DIR_APP) && cd $(DIR_SRC) && tar zvxf $(DIR_DL)/$(DL_FILE) > + cd $(DIR_APP) && patch -Np1 -i $(DIR_SRC)/src/patches/squid/squid-asnbl-0= .2.4_initialise_global_variables_to_make_compatible_with_python_3.patch > =20 > # Install ASNBL helper script > cd $(DIR_APP) && install -o root -g root -m 0755 asnbl-helper.py /usr/bi= n/asnbl-helper.py > diff --git a/src/patches/squid/squid-asnbl-0.2.4_initialise_global_variable= s_to_make_compatible_with_python_3.patch b/src/patches/squid/squid-asnbl-0.2.= 4_initialise_global_variables_to_make_compatible_with_python_3.patch > new file mode 100644 > index 000000000..e540d4e76 > --- /dev/null > +++ b/src/patches/squid/squid-asnbl-0.2.4_initialise_global_variables_to_ma= ke_compatible_with_python_3.patch > @@ -0,0 +1,100 @@ > +--- squid-asnbl-0.2.4/asnbl-helper_orig.py > ++++ squid-asnbl-0.2.4/asnbl-helper.py > +@@ -172,17 +172,19 @@ > + return parsedasns > + > + > +-def resolve_asn(ipaddr: str, asndb): > +- """ Function call: resolve_asn(IP address to be resolved, > +- ASN database instance object) > +- This function looks up the Autonomous System for the given IP address= . It expects > +- an IPFire location database object to be passed as a second parameter= , hence relying > +- on another function to set that up. """ > ++def resolve_asn(ipaddr: str): > ++ """ Function call: resolve_asn(IP address to be resolved) > ++ This function looks up the Autonomous System for the given IP address= . """ > ++ > ++ # Fix for #13023 > ++ # Initialize the result variable before it's first use, otherwise pyt= hon3 > ++ # will sometimes detect a 'mismatch' using global and local variables > ++ lookup_result =3D None > + > + # libloc cannot handle ipaddress objects here, so casting into a stri= ng is necessary > + # for good measure, to avoid exceptions here... > + try: > +- result =3D asndb.lookup(str(ipaddr)) > ++ lookup_result =3D ASNDB.lookup(str(ipaddr)) > + except BlockingIOError: > + # XXX: Prevent likely libloc bug from causing this helper to crash > + # (see upstream bug https://bugzilla.ipfire.org/show_bug.cgi?id= =3D13023) > +@@ -190,21 +192,25 @@ > + > + # In case nothing was returned above, satisfy result expectation to t= his function... > + try: > +- if not result.asn: > ++ if not lookup_result.asn: > + return 0 > + except AttributeError: > + return 0 > + > +- return result.asn > +- > +- > +-def asndb_response_tests(testdata: str, asndb): > +- """ Function call: asndb_response_tests(response rest data, > +- ASN database instance object) > ++ return lookup_result.asn > ++ > ++ > ++def asndb_response_tests(testdata: str): > ++ """ Function call: asndb_response_tests(response rest data) > + > + This function asserts the given ASN database to return expected ASNs = for > + given IP addresses in order to be considered operational. It returns > + True if this test succeeds, and False otherwise. """ > ++ > ++ # Fix for #13023 > ++ # Initialize the result variable before it's first use, otherwise pyt= hon3 > ++ # will sometimes detect a 'mismatch' using global and local variables > ++ lookup_result_test =3D None > + > + tresult =3D True > + > +@@ -216,13 +222,13 @@ > + > + for stestdata in ptdata: > + LOGIT.debug("Running response test for '%s' against ASNDB '%s' ..= .", > +- stestdata, asndb) > +- > +- returndata =3D resolve_asn(stestdata[0], asndb) > +- > +- if returndata !=3D int(stestdata[1]): > ++ stestdata, ASNDB) > ++ > ++ lookup_result_test =3D resolve_asn(stestdata[0]) > ++ > ++ if lookup_result_test !=3D int(stestdata[1]): > + LOGIT.error("Response test failed for ASNDB '%s' (tuple: %s),= aborting", > +- asndb, stestdata) > ++ ASNDB, stestdata) > + tresult =3D False > + break > + > +@@ -428,7 +434,7 @@ > + ASNDB =3D set_up_location_database(config["GENERAL"]["ASNDB_PATH"]) > + > + LOGIT.debug("Running ASN database response tests...") > +-if asndb_response_tests(config["GENERAL"]["TESTDATA"], ASNDB): > ++if asndb_response_tests(config["GENERAL"]["TESTDATA"]): > + LOGIT.debug("ASN database operational - excellent. Waiting for input.= ..") > + else: > + LOGIT.error("ASN database response tests failed, aborting") > +@@ -490,7 +496,7 @@ > + ASNS =3D [] > + for singleip in IPS: > + # Enumerate ASN for this IP address... > +- resolvedasn =3D resolve_asn(singleip, ASNDB) > ++ resolvedasn =3D resolve_asn(singleip) > + > + # In case protection against destinations without public AS annou= ncements for their > + # IP addresses is desired, the query will be denied in case ASN = =3D 0 appears in an --=20 Sent from my laptop --===============1439059577391498134==--