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: 7223 bytes --]

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