From mboxrd@z Thu Jan  1 00:00:00 1970
From: Peter =?utf-8?q?M=C3=BCller?= <peter.mueller@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 19:29:00 +0000
Message-ID: <02c72198-ac55-e855-1f6e-3e0dbe200364@ipfire.org>
In-Reply-To: <f6909803-e605-7e3c-92e1-25e0f3159eec@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============4970129495297814800=="
List-Id: <development.lists.ipfire.org>

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

Hello Adolf,

thank you for your e-mail, and apologies for having dropped the
ball on this. I was under the impression that upstream would work
on a fix, but now see this has not materialized.

I'll merge the patch for Core Update 177 and ping upstream again.

Apologies for my tardy reaction.

Thanks, and best regards,
Peter M=C3=BCller


> Hi All,
> This patch has now been waiting for 3 months and missed 3 or 4 Core Updates=
 and my last reminder was not responded to.
>=20
> If there is a problem with this patch fix let me know, or provide input int=
o the bug report so that it can be worked on.
>=20
> Regards,
> Adolf.
>=20
> 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 problem =
from the bug.
>>> =C2=A0=C2=A0=C2=A0 First I discovered that the helper only sometimes thro=
wing the error and quits even
>>> =C2=A0=C2=A0=C2=A0 for the same values and queries. Also the timespan unt=
il the error happens was quite
>>> =C2=A0=C2=A0=C2=A0 different for every restart of squid=C2=A0 (minutes to=
 hours). And it does not depend on
>>> =C2=A0=C2=A0=C2=A0 the traffic on the proxy, even one connection could ca=
use a crash while ten or
>>> =C2=A0=C2=A0=C2=A0 hundrets won't. After a few days of testing different =
solutions and done a lot of
>>> =C2=A0=C2=A0=C2=A0 debugging, redesigning the function did not fully solv=
e the problem. Such standard
>>> =C2=A0=C2=A0=C2=A0 things like checking the result variable for NULL (or =
it's equivalent "is None" in
>>> =C2=A0=C2=A0=C2=A0 python) before evaluating it's subfunction produces th=
e exact same error message. But
>>> =C2=A0=C2=A0=C2=A0 with that knowledge it more and more turns out that py=
thon3 sometimes 'detects' the
>>> =C2=A0=C2=A0=C2=A0 local return variable if it was a misused global. So f=
or a full fix, the return
>>> =C2=A0=C2=A0=C2=A0 variable also has to be initialized that python3 won't=
 detect 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 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 src/patches/squid/squid-asnbl-0.2.4_initialise_=
global_variables_to_make_compatible_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 $(DIR_DL)/$(DL_FILE)
>>> +=C2=A0=C2=A0=C2=A0 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
>>> =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 asnbl-helper.py /usr/bin/asnbl-helper.py
>>> diff --git a/src/patches/squid/squid-asnbl-0.2.4_initialise_global_variab=
les_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_=
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 reso=
lved,
>>> +-=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 ins=
tance object)
>>> +-=C2=A0=C2=A0=C2=A0 This function looks up the Autonomous System for the=
 given IP address. It expects
>>> +-=C2=A0=C2=A0=C2=A0 an IPFire location database object to be passed as a=
 second 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 reso=
lved)
>>> ++=C2=A0=C2=A0=C2=A0 This function looks up the Autonomous System for the=
 given IP address. """
>>> ++
>>> ++=C2=A0=C2=A0=C2=A0 # Fix for #13023
>>> ++=C2=A0=C2=A0=C2=A0 # Initialize the result variable before it's first u=
se, otherwise python3
>>> ++=C2=A0=C2=A0=C2=A0 # will sometimes detect a 'mismatch' using global an=
d local 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, =
so casting into 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(=
ipaddr))
>>> ++=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 lookup_result =3D ASNDB.look=
up(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 l=
ibloc bug from causing this helper to crash
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # (see upstream bug htt=
ps://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 r=
esult 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 res=
t 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 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 res=
t data)
>>> +
>>> +=C2=A0=C2=A0=C2=A0=C2=A0 This function asserts the given ASN database to=
 return expected ASNs for
>>> +=C2=A0=C2=A0=C2=A0=C2=A0 given IP addresses in order to be considered op=
erational. It 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 u=
se, otherwise python3
>>> ++=C2=A0=C2=A0=C2=A0 # will sometimes detect a 'mismatch' using global an=
d local 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 re=
sponse test for '%s' against ASNDB '%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(s=
testdata[0], asndb)
>>> +-
>>> +-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if returndata !=3D int(stest=
data[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 resol=
ve_asn(stestdata[0])
>>> ++
>>> ++=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if lookup_result_test !=3D i=
nt(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' (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, =
stestdata)
>>> ++=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, =
stestdata)
>>> +=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 - excelle=
nt. Waiting for 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 thi=
s IP address...
>>> +-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 resolvedasn =3D resolve_asn(=
singleip, ASNDB)
>>> ++=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 resolvedasn =3D resolve_asn(=
singleip)
>>> +
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # In case protection ag=
ainst destinations without public AS announcements for their
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # IP addresses is desir=
ed, the query will be denied in case ASN =3D 0 appears in an
>>
>=20

--===============4970129495297814800==--