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

[Adolf Belka <adolf.belka@ipfire.org>]

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

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
>

-- 
Sent from my laptop