[PATCH saem_ref] [ark] Avoid infinite loops by settings a maximum number of iterations limit

Philippe Pepiot philippe.pepiot at logilab.fr
Wed Feb 28 11:35:08 CET 2018


On 28/02/2018, Denis Laxalde wrote:
> # HG changeset patch
> # User Denis Laxalde <denis.laxalde at logilab.fr>
> # Date 1519634988 -3600
> #      Mon Feb 26 09:49:48 2018 +0100
> # Node ID 69c8586db0b8c985b70d5e8a7d7a7da5e1d57b8d
> # Parent  dc88b0ed93f8b907fdf9e7437388af494a55a9df
> # Available At http://hg.logilab.org/review/cubes/saem_ref
> #              hg pull http://hg.logilab.org/review/cubes/saem_ref -r 69c8586db0b8
> [ark] Avoid infinite loops by settings a maximum number of iterations limit
> 
> Arbitrarily set this limit to 20, without further usage knowledge.
> We don't catch the error on application side for now, waiting for an
> actual use case.
> 
> diff --git a/cubicweb_saem_ref/ark.py b/cubicweb_saem_ref/ark.py
> --- a/cubicweb_saem_ref/ark.py
> +++ b/cubicweb_saem_ref/ark.py
> @@ -20,6 +20,7 @@ import re
>  from six import text_type
>  
>  
> +ARK_MAXIT = 20

Not sure we want to expose this implementation detail in the python
code.

>  ARK_PREFIX = 'rf'
>  ARK_CONTROLCHAR = 'g'
>  ARK_NAME_LENGTH = 10
> @@ -57,8 +58,8 @@ def generate_ark(cnx, naan):
>      Return the generated name.
>      """
>      cu = cnx.system_sql(
> -        'select * from gen_ark(%s, %s, %s, %s);',
> -        (naan, ARK_NAME_LENGTH, ARK_PREFIX, ARK_CONTROLCHAR),
> +        'select * from gen_ark(%s, %s, %s, %s, %s);',
> +        (naan, ARK_NAME_LENGTH, ARK_PREFIX, ARK_CONTROLCHAR, ARK_MAXIT),
>      )
>      name, = cu.fetchone()
>      return name
> @@ -71,8 +72,8 @@ def generate_qualified_ark(cnx, naan, na
>      Return the generated qualifier.
>      """
>      cu = cnx.system_sql(
> -        'select * from gen_qualified_ark(%s, %s, %s);',
> -        (naan, name, ARK_QUALIFIER_LENGTH),
> +        'select * from gen_qualified_ark(%s, %s, %s, %s);',
> +        (naan, name, ARK_QUALIFIER_LENGTH, ARK_MAXIT),
>      )
>      qualifier, = cu.fetchone()
>      return qualifier
> diff --git a/cubicweb_saem_ref/schema/ark.postgres.sql b/cubicweb_saem_ref/schema/ark.postgres.sql
> --- a/cubicweb_saem_ref/schema/ark.postgres.sql
> +++ b/cubicweb_saem_ref/schema/ark.postgres.sql
> @@ -52,12 +52,17 @@ END;
>  -- Insert a record in "ark" table ensuring uniqueness constraints are
>  -- fulfilled.
>  CREATE OR REPLACE
> -FUNCTION gen_ark(naan INTEGER, len INTEGER, prefix TEXT, control_char TEXT)
> +FUNCTION gen_ark(naan INTEGER, len INTEGER, prefix TEXT, control_char TEXT, maxit INTEGER)

Maybe you want a default value for maxit (maxit INTEGER DEFAULT 20) ?

Not sure we handle migrations yet but take care that plsql functions can
have multiple signatures and definitions so CREATE OR REPLACE here will
not drop the old function definition (without the maxit param).
You (will) have to DROP FUNCTION IF EXISTS to ensure the old definition does
not exists.

>  RETURNS TEXT AS $$
>  DECLARE
>      ark_name TEXT;
> +    nit INTEGER;
>  BEGIN
> +    nit := 0;
>      LOOP
> +        ASSERT nit < maxit,
> +            'ark generation loop exceeded maximum number of iterations: ' || maxit;
> +        nit := nit + 1;
>          BEGIN
>              INSERT INTO ark
>                  VALUES (naan, gen_ark_part(len, prefix, control_char), DEFAULT)
> @@ -66,7 +71,6 @@ BEGIN
>          EXCEPTION
>              WHEN unique_violation THEN
>                  -- Continue and try with another "name".
> -                RAISE WARNING 'unique_violation';
>                  NULL;
>          END;
>      END LOOP;
> @@ -76,17 +80,22 @@ END;
>  -- Insert a record in "ark" table from a "naan" value and a "base" identifier
>  -- using a qualifier.
>  CREATE OR REPLACE
> -FUNCTION gen_qualified_ark(naan_ INTEGER, base TEXT, len INTEGER)
> +FUNCTION gen_qualified_ark(naan_ INTEGER, base TEXT, len INTEGER, maxit INTEGER)
>  RETURNS TEXT AS $$
>  DECLARE
>      ark_qualifier TEXT;
> +    nit INTEGER;
>  BEGIN
>      PERFORM 1 FROM ark WHERE ark.name = base AND ark.qualifier = '';
>      IF NOT FOUND THEN
>          RAISE 'no ark record matching "%/%" found', naan_, base
>          USING ERRCODE = 'invalid_parameter_value';
>      END IF;
> +    nit := 0;
>      LOOP
> +        ASSERT nit < maxit,
> +            'ark generation loop exceeded maximum number of iterations: ' || maxit;
> +        nit := nit + 1;
>          BEGIN
>              INSERT INTO ark
>                  VALUES (naan_, base, gen_ark_part(len, '', ''))
> @@ -95,7 +104,6 @@ BEGIN
>          EXCEPTION
>              WHEN unique_violation THEN
>                  -- Continue and try with another "name".
> -                RAISE WARNING 'unique_violation';
>                  NULL;
>          END;
>      END LOOP;
> diff --git a/test/test_ark.py b/test/test_ark.py
> --- a/test/test_ark.py
> +++ b/test/test_ark.py
> @@ -18,6 +18,7 @@
>  from unittest import TestCase
>  
>  import psycopg2
> +import psycopg2.errorcodes
>  
>  from cubicweb.devtools import (
>      PostgresApptestConfiguration,
> @@ -115,8 +116,8 @@ class ArkServiceTC(testlib.CubicWebTC):
>          with self.connect() as conn:
>              with conn.cursor() as cu:
>                  for _ in range(n):
> -                    cu.execute("SELECT * from gen_ark(%s, %s, %s, %s);",
> -                               (0, 4, 'rf', 'x'))
> +                    cu.execute("SELECT * from gen_ark(%s, %s, %s, %s, %s);",
> +                               (0, 4, 'rf', 'x', 10))
>                      arks.append(cu.fetchone()[0])
>                      conn.commit()
>                  cu.execute("SELECT * from ark")
> @@ -124,6 +125,33 @@ class ArkServiceTC(testlib.CubicWebTC):
>          self.assertEqual(len(set(r)), n, len(r))
>          self.assertCountEqual([ark for _, ark, _ in r], arks)
>  
> +    def test_gen_ark_maxit(self):
> +        with self.connect() as conn:
> +            with conn.cursor() as cu:
> +                with self.assertRaises(psycopg2.InternalError) as cm:
> +                    cu.execute("SELECT * from gen_ark(%s, %s, %s, %s, %s);",
> +                               (0, 4, 'rf', 'x', 0))
> +                exc = cm.exception
> +                self.assertEqual(psycopg2.errorcodes.lookup(exc.pgcode),
> +                                 'ASSERT_FAILURE')
> +                self.assertIn('maximum number of iterations: 0',
> +                              str(exc))
> +
> +    def test_gen_qualified_ark_maxit(self):
> +        with self.connect() as conn:
> +            with conn.cursor() as cu:
> +                cu.execute("INSERT INTO ark VALUES (%s, %s, DEFAULT);",
> +                           (0, "parent", ))
> +                conn.commit()
> +                with self.assertRaises(psycopg2.InternalError) as cm:
> +                    cu.execute("SELECT * from gen_qualified_ark(%s, %s, %s, %s);",
> +                               (0, "parent", 7, 0))
> +                exc = cm.exception
> +                self.assertEqual(psycopg2.errorcodes.lookup(exc.pgcode),
> +                                 'ASSERT_FAILURE')
> +                self.assertIn('maximum number of iterations: 0',
> +                              str(exc))
> +
>      def test_qualifier(self):
>          def insert(qualifier=None):
>              with self.connect() as conn:
> @@ -156,7 +184,7 @@ class ArkServiceTC(testlib.CubicWebTC):
>          with self.connect() as conn:
>              with conn.cursor() as cu:
>                  with self.assertRaises(psycopg2.DataError) as cm:
> -                    cu.execute("SELECT * FROM gen_qualified_ark(%s, %s, %s);",
> +                    cu.execute("SELECT * FROM gen_qualified_ark(%s, %s, %s, 20);",
>                                 (1, "doesnotexist", 3))
>          self.assertIn('no ark record matching "1/doesnotexist" found',
>                        str(cm.exception))
> @@ -166,7 +194,7 @@ class ArkServiceTC(testlib.CubicWebTC):
>              with conn.cursor() as cu:
>                  cu.execute("INSERT INTO ark VALUES (%s, %s, DEFAULT);",
>                             (12345, "exists", ))
> -                cu.execute("SELECT * FROM gen_qualified_ark(%s, %s, %s);",
> +                cu.execute("SELECT * FROM gen_qualified_ark(%s, %s, %s, 20);",
>                             (12345, "exists", 3))
>                  cu.execute("SELECT * FROM ark"
>                             " WHERE ark.naan = 12345 AND ark.name = 'exists'"
> 



More information about the saem-devel mailing list