[Tarantool-patches] [PATCH] sql: fix off-by-one error in QP

[Tarantool-patches] [PATCH] sql: fix off-by-one error in QP

[Nikita Pettik]

In scope of 8fac697 it was fixed misbehavior while passing floating
point values to integer iterator. Unfortunately, that patch introduced
off-by-one error. Number of constraints (equalities and inequalities)
can not be greater than number of key parts (obviously, each constraint
can be tested against at most one index part). Inequality constraint can
involve only field representing last key part. To get it number of
constraints was used as index. However, since array is 0-based, the last
key part should be calculated as parts[eq_numb - 1]. Before this patch
it was parts[eq_numb].

Closes #4558
---
Branch: https://github.com/tarantool/tarantool/commits/np/gh-4558-off-by-one-qp-fix
Issue: https://github.com/tarantool/tarantool/issues/4558

 src/box/sql/wherecode.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index de51c0ba5..5bc27f134 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1118,10 +1118,20 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
 			}
 		}
 		/* Inequality constraint comes always at the end of list. */
-		enum field_type ineq_type = idx_def->key_def->parts[nEq].type;
-		if (pRangeStart != NULL && (ineq_type == FIELD_TYPE_INTEGER ||
-					    ineq_type == FIELD_TYPE_UNSIGNED))
-			force_integer_reg = regBase + nEq;
+		part_count = idx_def->key_def->part_count;
+		if (pRangeStart != NULL) {
+			/*
+			 * nEq == 0 means that filter condition
+			 * contains only inequality.
+			 */
+			uint32_t ineq_idx = nEq == 0 ? 0 : nEq - 1;
+			assert(ineq_idx < part_count);
+			enum field_type ineq_type =
+				idx_def->key_def->parts[ineq_idx].type;
+			if (ineq_type == FIELD_TYPE_INTEGER ||
+			    ineq_type == FIELD_TYPE_UNSIGNED)
+				force_integer_reg = regBase + nEq;
+		}
 		emit_apply_type(pParse, regBase, nConstraint - bSeekPastNull,
 				start_types);
 		if (pLoop->nSkip > 0 && nConstraint == pLoop->nSkip) {
-- 
2.15.1

Re: [Tarantool-patches] [PATCH] sql: fix off-by-one error in QP

[Vladislav Shpilevoy]

Hi! Thanks for the patch! LGTM.

On 21/10/2019 18:45, Nikita Pettik wrote:
> In scope of 8fac697 it was fixed misbehavior while passing floating
> point values to integer iterator. Unfortunately, that patch introduced
> off-by-one error. Number of constraints (equalities and inequalities)
> can not be greater than number of key parts (obviously, each constraint
> can be tested against at most one index part). Inequality constraint can
> involve only field representing last key part. To get it number of
> constraints was used as index. However, since array is 0-based, the last
> key part should be calculated as parts[eq_numb - 1]. Before this patch
> it was parts[eq_numb].
> 
> Closes #4558
> ---
> Branch: https://github.com/tarantool/tarantool/commits/np/gh-4558-off-by-one-qp-fix
> Issue: https://github.com/tarantool/tarantool/issues/4558
> 
>  src/box/sql/wherecode.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 

Re: [Tarantool-patches] [PATCH] sql: fix off-by-one error in QP

[Nikita Pettik]

On 23 Oct 23:12, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch! LGTM.

Pushed to master.
 
> On 21/10/2019 18:45, Nikita Pettik wrote:
> > In scope of 8fac697 it was fixed misbehavior while passing floating
> > point values to integer iterator. Unfortunately, that patch introduced
> > off-by-one error. Number of constraints (equalities and inequalities)
> > can not be greater than number of key parts (obviously, each constraint
> > can be tested against at most one index part). Inequality constraint can
> > involve only field representing last key part. To get it number of
> > constraints was used as index. However, since array is 0-based, the last
> > key part should be calculated as parts[eq_numb - 1]. Before this patch
> > it was parts[eq_numb].
> > 
> > Closes #4558
> > ---
> > Branch: https://github.com/tarantool/tarantool/commits/np/gh-4558-off-by-one-qp-fix
> > Issue: https://github.com/tarantool/tarantool/issues/4558
> > 
> >  src/box/sql/wherecode.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> > 

Re: [Tarantool-patches] [PATCH] sql: fix off-by-one error in QP

[Alexander Turenko]

On Thu, Oct 24, 2019 at 11:13:47AM +0300, Nikita Pettik wrote:
> On 23 Oct 23:12, Vladislav Shpilevoy wrote:
> > Hi! Thanks for the patch! LGTM.
> 
> Pushed to master.

8fac697 is 2.2.0-543-g8fac69721, so current 2.2 branch possibly affected
by the issue. Shouldn't this commit be cherry picked to 2.2?

>  
> > On 21/10/2019 18:45, Nikita Pettik wrote:
> > > In scope of 8fac697 it was fixed misbehavior while passing floating
> > > point values to integer iterator. Unfortunately, that patch introduced
> > > off-by-one error. Number of constraints (equalities and inequalities)
> > > can not be greater than number of key parts (obviously, each constraint
> > > can be tested against at most one index part). Inequality constraint can
> > > involve only field representing last key part. To get it number of
> > > constraints was used as index. However, since array is 0-based, the last
> > > key part should be calculated as parts[eq_numb - 1]. Before this patch
> > > it was parts[eq_numb].
> > > 
> > > Closes #4558
> > > ---
> > > Branch: https://github.com/tarantool/tarantool/commits/np/gh-4558-off-by-one-qp-fix
> > > Issue: https://github.com/tarantool/tarantool/issues/4558
> > > 
> > >  src/box/sql/wherecode.c | 18 ++++++++++++++----
> > >  1 file changed, 14 insertions(+), 4 deletions(-)
> > > 

Re: [Tarantool-patches] [PATCH] sql: fix off-by-one error in QP

[Nikita Pettik]

On 30 Dec 01:20, Alexander Turenko wrote:
> On Thu, Oct 24, 2019 at 11:13:47AM +0300, Nikita Pettik wrote:
> > On 23 Oct 23:12, Vladislav Shpilevoy wrote:
> > > Hi! Thanks for the patch! LGTM.
> > 
> > Pushed to master.
> 
> 8fac697 is 2.2.0-543-g8fac69721, so current 2.2 branch possibly affected
> by the issue. Shouldn't this commit be cherry picked to 2.2?

Yep, let's cherry-pick it to 2.2 (ticket had no milestone at all, so
probably I forgot to promote it to 2.2).

> > > On 21/10/2019 18:45, Nikita Pettik wrote:
> > > > In scope of 8fac697 it was fixed misbehavior while passing floating
> > > > point values to integer iterator. Unfortunately, that patch introduced
> > > > off-by-one error. Number of constraints (equalities and inequalities)
> > > > can not be greater than number of key parts (obviously, each constraint
> > > > can be tested against at most one index part). Inequality constraint can
> > > > involve only field representing last key part. To get it number of
> > > > constraints was used as index. However, since array is 0-based, the last
> > > > key part should be calculated as parts[eq_numb - 1]. Before this patch
> > > > it was parts[eq_numb].
> > > > 
> > > > Closes #4558
> > > > ---
> > > > Branch: https://github.com/tarantool/tarantool/commits/np/gh-4558-off-by-one-qp-fix
> > > > Issue: https://github.com/tarantool/tarantool/issues/4558
> > > > 
> > > >  src/box/sql/wherecode.c | 18 ++++++++++++++----
> > > >  1 file changed, 14 insertions(+), 4 deletions(-)
> > > >