Re: [PATCH] squid-asnbl: Fix for bug#13023 - squid-asnbl-helper segfaulting and shutdown squid

["Peter Müller" <peter.mueller@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 8754 bytes --]

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üller


> 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.
> 
> If there is a problem with this patch fix let me know, or provide input 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 problem from the bug.
>>>     First I discovered that the helper only sometimes throwing the error and quits even
>>>     for the same values and queries. Also the timespan until the error happens was quite
>>>     different for every restart of squid  (minutes to hours). And it does not 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 done a lot of
>>>     debugging, redesigning the function did not fully solve the problem. Such 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 error 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, the return
>>>     variable also has to be initialized that python3 won't detect it's usage 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 squid-asnbl.
>>>
>>> Fixes: Bug#13023
>>> Tested-by: Nicolas Pӧhlmann <business(a)hardcoretec.com>
>>> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>> ---
>>>   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
>>>
>>> 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
>>>         # Install ASNBL helper script
>>>       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_variables_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 @@
>>> +     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 python3
>>> ++    # will sometimes detect a 'mismatch' using global and local variables
>>> ++    lookup_result = None
>>> +
>>> +     # libloc cannot handle ipaddress objects here, so casting into a string is necessary
>>> +     # for good measure, to avoid exceptions here...
>>> +     try:
>>> +-        result = asndb.lookup(str(ipaddr))
>>> ++        lookup_result = 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=13023)
>>> +@@ -190,21 +192,25 @@
>>> +
>>> +     # In case nothing was returned above, satisfy result expectation to this 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 python3
>>> ++    # will sometimes detect a 'mismatch' using global and local variables
>>> ++    lookup_result_test = None
>>> +
>>> +     tresult = True
>>> +
>>> +@@ -216,13 +222,13 @@
>>> +
>>> +     for stestdata in ptdata:
>>> +         LOGIT.debug("Running response test for '%s' against ASNDB '%s' ...",
>>> +-                    stestdata, asndb)
>>> +-
>>> +-        returndata = resolve_asn(stestdata[0], asndb)
>>> +-
>>> +-        if returndata != int(stestdata[1]):
>>> ++                    stestdata, ASNDB)
>>> ++
>>> ++        lookup_result_test = resolve_asn(stestdata[0])
>>> ++
>>> ++        if lookup_result_test != int(stestdata[1]):
>>> +             LOGIT.error("Response test failed for ASNDB '%s' (tuple: %s), aborting",
>>> +-                        asndb, stestdata)
>>> ++                        ASNDB, stestdata)
>>> +             tresult = False
>>> +             break
>>> +
>>> +@@ -428,7 +434,7 @@
>>> + ASNDB = 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 = []
>>> +     for singleip in IPS:
>>> +         # Enumerate ASN for this IP address...
>>> +-        resolvedasn = resolve_asn(singleip, ASNDB)
>>> ++        resolvedasn = resolve_asn(singleip)
>>> +
>>> +         # In case protection against destinations without public AS announcements for their
>>> +         # IP addresses is desired, the query will be denied in case ASN = 0 appears in an
>>
>