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

Denis Laxalde denis.laxalde at logilab.fr
Wed Feb 28 12:22:36 CET 2018


Philippe Pepiot a écrit :
> 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.

Well, I'm not sure I want to hide it either :) In particular, I
anticipate that these globals may turn into instance configuration
options at some point.


>>  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) ?

I didn't know this was possible, thanks.

> 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.

So, I think I'll keep the actual implementation to avoid getting in such
troubles *now*.

Thanks a lot for the review.

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