From mboxrd@z Thu Jan  1 00:00:00 1970
From: Adolf Belka <adolf.belka@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH] squid-asnbl: Fix for bug#13023 - squid-asnbl-helper
 segfaulting and shutdown squid
Date: Sat, 01 Jul 2023 18:04:40 +0200
Message-ID: <f6909803-e605-7e3c-92e1-25e0f3159eec@ipfire.org>
In-Reply-To: <a6651e7a-6d62-c079-1d87-b6aa5ff49cde@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============5312191910905640292=="
List-Id: <development.lists.ipfire.org>

--===============5312191910905640292==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hi All,
This patch has now been waiting for 3 months and missed 3 or 4 Core=20
Updates and my last reminder was not responded to.

If there is a problem with this patch fix let me know, or provide input=20
into the bug report so that it can be worked on.

Regards,
Adolf.

On 17/05/2023 14:37, Adolf Belka wrote:
> 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=20
>> problem from the bug.
>> =C2=A0=C2=A0=C2=A0 First I discovered that the helper only sometimes throw=
ing the=20
>> error and quits even
>> =C2=A0=C2=A0=C2=A0 for the same values and queries. Also the timespan unti=
l the=20
>> error happens was quite
>> =C2=A0=C2=A0=C2=A0 different for every restart of squid=C2=A0 (minutes to =
hours). And it=20
>> does not depend on
>> =C2=A0=C2=A0=C2=A0 the traffic on the proxy, even one connection could cau=
se a crash=20
>> while ten or
>> =C2=A0=C2=A0=C2=A0 hundrets won't. After a few days of testing different s=
olutions=20
>> and done a lot of
>> =C2=A0=C2=A0=C2=A0 debugging, redesigning the function did not fully solve=
 the=20
>> problem. Such standard
>> =C2=A0=C2=A0=C2=A0 things like checking the result variable for NULL (or i=
t's=20
>> equivalent "is None" in
>> =C2=A0=C2=A0=C2=A0 python) before evaluating it's subfunction produces the=
 exact=20
>> same error message. But
>> =C2=A0=C2=A0=C2=A0 with that knowledge it more and more turns out that pyt=
hon3=20
>> sometimes 'detects' the
>> =C2=A0=C2=A0=C2=A0 local return variable if it was a misused global. So fo=
r a full=20
>> fix, the return
>> =C2=A0=C2=A0=C2=A0 variable also has to be initialized that python3 won't =
detect=20
>> it's usage as an
>> =C2=A0=C2=A0=C2=A0 '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=20
>> for squid-asnbl.
>>
>> Fixes: Bug#13023
>> Tested-by: Nicolas P=D3=A7hlmann <business(a)hardcoretec.com>
>> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
>> ---
>> =C2=A0 lfs/squid-asnbl=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0 1 +
>> =C2=A0 ...les_to_make_compatible_with_python_3.patch | 100 +++++++++++++++=
+++
>> =C2=A0 2 files changed, 101 insertions(+)
>> =C2=A0 create mode 100644=20
>> src/patches/squid/squid-asnbl-0.2.4_initialise_global_variables_to_make_co=
mpatible_with_python_3.patch
>>
>> 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)) :
>> =C2=A0 $(TARGET) : $(patsubst %,$(DIR_DL)/%,$(objects))
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 @$(PREBUILD)
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 @rm -rf $(DIR_APP) && cd $(DIR_SRC) && tar =
zvxf=20
>> $(DIR_DL)/$(DL_FILE)
>> +=C2=A0=C2=A0=C2=A0 cd $(DIR_APP) && patch -Np1 -i=20
>> $(DIR_SRC)/src/patches/squid/squid-asnbl-0.2.4_initialise_global_variables=
_to_make_compatible_with_python_3.patch
>> =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # Install ASNBL helper script
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 cd $(DIR_APP) && install -o root -g root -m=
 0755=20
>> asnbl-helper.py /usr/bin/asnbl-helper.py
>> diff --git=20
>> a/src/patches/squid/squid-asnbl-0.2.4_initialise_global_variables_to_make_=
compatible_with_python_3.patch=20
>> b/src/patches/squid/squid-asnbl-0.2.4_initialise_global_variables_to_make_=
compatible_with_python_3.patch=20
>>
>> new file mode 100644
>> index 000000000..e540d4e76
>> --- /dev/null
>> +++=20
>> b/src/patches/squid/squid-asnbl-0.2.4_initialise_global_variables_to_make_=
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 @@
>> +=C2=A0=C2=A0=C2=A0=C2=A0 return parsedasns
>> +
>> +
>> +-def resolve_asn(ipaddr: str, asndb):
>> +-=C2=A0=C2=A0=C2=A0 """ Function call: resolve_asn(IP address to be resol=
ved,
>> +-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ASN database instan=
ce object)
>> +-=C2=A0=C2=A0=C2=A0 This function looks up the Autonomous System for the =
given IP=20
>> address. It expects
>> +-=C2=A0=C2=A0=C2=A0 an IPFire location database object to be passed as a =
second=20
>> parameter, hence relying
>> +-=C2=A0=C2=A0=C2=A0 on another function to set that up. """
>> ++def resolve_asn(ipaddr: str):
>> ++=C2=A0=C2=A0=C2=A0 """ Function call: resolve_asn(IP address to be resol=
ved)
>> ++=C2=A0=C2=A0=C2=A0 This function looks up the Autonomous System for the =
given IP=20
>> address. """
>> ++
>> ++=C2=A0=C2=A0=C2=A0 # Fix for #13023
>> ++=C2=A0=C2=A0=C2=A0 # Initialize the result variable before it's first us=
e,=20
>> otherwise python3
>> ++=C2=A0=C2=A0=C2=A0 # will sometimes detect a 'mismatch' using global and=
 local=20
>> variables
>> ++=C2=A0=C2=A0=C2=A0 lookup_result =3D None
>> +
>> +=C2=A0=C2=A0=C2=A0=C2=A0 # libloc cannot handle ipaddress objects here, s=
o casting into=20
>> a string is necessary
>> +=C2=A0=C2=A0=C2=A0=C2=A0 # for good measure, to avoid exceptions here...
>> +=C2=A0=C2=A0=C2=A0=C2=A0 try:
>> +-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 result =3D asndb.lookup(str(i=
paddr))
>> ++=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 lookup_result =3D ASNDB.looku=
p(str(ipaddr))
>> +=C2=A0=C2=A0=C2=A0=C2=A0 except BlockingIOError:
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # XXX: Prevent likely li=
bloc bug from causing this helper=20
>> to crash
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # (see upstream bug=20
>> https://bugzilla.ipfire.org/show_bug.cgi?id=3D13023)
>> +@@ -190,21 +192,25 @@
>> +
>> +=C2=A0=C2=A0=C2=A0=C2=A0 # In case nothing was returned above, satisfy re=
sult=20
>> expectation to this function...
>> +=C2=A0=C2=A0=C2=A0=C2=A0 try:
>> +-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if not result.asn:
>> ++=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if not lookup_result.asn:
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
return 0
>> +=C2=A0=C2=A0=C2=A0=C2=A0 except AttributeError:
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return 0
>> +
>> +-=C2=A0=C2=A0=C2=A0 return result.asn
>> +-
>> +-
>> +-def asndb_response_tests(testdata: str, asndb):
>> +-=C2=A0=C2=A0=C2=A0 """ Function call: asndb_response_tests(response rest=
 data,
>> +-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ASN database instance=20
>> object)
>> ++=C2=A0=C2=A0=C2=A0 return lookup_result.asn
>> ++
>> ++
>> ++def asndb_response_tests(testdata: str):
>> ++=C2=A0=C2=A0=C2=A0 """ Function call: asndb_response_tests(response rest=
 data)
>> +
>> +=C2=A0=C2=A0=C2=A0=C2=A0 This function asserts the given ASN database to =
return expected=20
>> ASNs for
>> +=C2=A0=C2=A0=C2=A0=C2=A0 given IP addresses in order to be considered ope=
rational. It=20
>> returns
>> +=C2=A0=C2=A0=C2=A0=C2=A0 True if this test succeeds, and False otherwise.=
 """
>> ++
>> ++=C2=A0=C2=A0=C2=A0 # Fix for #13023
>> ++=C2=A0=C2=A0=C2=A0 # Initialize the result variable before it's first us=
e,=20
>> otherwise python3
>> ++=C2=A0=C2=A0=C2=A0 # will sometimes detect a 'mismatch' using global and=
 local=20
>> variables
>> ++=C2=A0=C2=A0=C2=A0 lookup_result_test =3D None
>> +
>> +=C2=A0=C2=A0=C2=A0=C2=A0 tresult =3D True
>> +
>> +@@ -216,13 +222,13 @@
>> +
>> +=C2=A0=C2=A0=C2=A0=C2=A0 for stestdata in ptdata:
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 LOGIT.debug("Running res=
ponse test for '%s' against ASNDB=20
>> '%s' ...",
>> +-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 stestdata, asndb)
>> +-
>> +-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 returndata =3D resolve_asn(st=
estdata[0], asndb)
>> +-
>> +-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if returndata !=3D int(stestd=
ata[1]):
>> ++=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 stestdata, ASNDB)
>> ++
>> ++=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 lookup_result_test =3D resolv=
e_asn(stestdata[0])
>> ++
>> ++=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if lookup_result_test !=3D in=
t(stestdata[1]):
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
LOGIT.error("Response test failed for ASNDB '%s'=20
>> (tuple: %s), aborting",
>> +-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 asndb, ste=
stdata)
>> ++=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ASNDB, ste=
stdata)
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
tresult =3D False
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
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"]):
>> +=C2=A0=C2=A0=C2=A0=C2=A0 LOGIT.debug("ASN database operational - excellen=
t. Waiting for=20
>> input...")
>> + else:
>> +=C2=A0=C2=A0=C2=A0=C2=A0 LOGIT.error("ASN database response tests failed,=
 aborting")
>> +@@ -490,7 +496,7 @@
>> +=C2=A0=C2=A0=C2=A0=C2=A0 ASNS =3D []
>> +=C2=A0=C2=A0=C2=A0=C2=A0 for singleip in IPS:
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # Enumerate ASN for this=
 IP address...
>> +-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 resolvedasn =3D resolve_asn(s=
ingleip, ASNDB)
>> ++=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 resolvedasn =3D resolve_asn(s=
ingleip)
>> +
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # In case protection aga=
inst destinations without public AS=20
>> announcements for their
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # IP addresses is desire=
d, the query will be denied in case=20
>> ASN =3D 0 appears in an
>

--=20
Sent from my laptop


--===============5312191910905640292==--