Zend Framework

Zend_Db_Table->getRow() fails due to incorrect check in seek method

Details

  • Type: Bug Bug
  • Status: Resolved Resolved
  • Priority: Blocker Blocker
  • Resolution: Fixed
  • Affects Version/s: 1.9.6
  • Fix Version/s: 1.11.1
  • Component/s: Zend_Db_Table
  • Labels:
    None

Description

When using Zend_Db_Table for fetching data from certain row from DB table Zend Framework ncorrectly checks range of available/seekable positions in Zend_Db_Table_Rowset_Abstract::seek() on line 323 and throws exception "Illegal index is ...".

The solution is to change condition which triggers exception with fixing comparison from grather or equal to grather.

BEFORE:

if ($position < 0 || $position >= $this->_count) { ...

AFTER:

if ($position < 0 || $position > $this->_count) { ...

Attachments

  1. File
    Abstract.php
    19/May/10 11:36 AM
    39 kB
    kalyanraj
  2. File
    ZF-8486_unit-test.diff
    19/Feb/10 12:46 PM
    0.7 kB
    Ralph Schindler

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Stephan "Bladed" de Souza added a comment -

The issue still exists on 1.9.7, already checked!

Show
Stephan "Bladed" de Souza added a comment - The issue still exists on 1.9.7, already checked!
Hide
Permalink
Nicolas Grevet added a comment -

This happens when you apply a foreach (for example) on a rowset and then try to get a particular row of this same rowset. After the foreach, the Iterator index is left at the end, after the last element (max(index)+1). The getRow() method fails to seek back to the former index because it simply doesn't exists in the Iterator. Happens in 1.10.0.

Show
Nicolas Grevet added a comment - This happens when you apply a foreach (for example) on a rowset and then try to get a particular row of this same rowset. After the foreach, the Iterator index is left at the end, after the last element (max(index)+1). The getRow() method fails to seek back to the former index because it simply doesn't exists in the Iterator. Happens in 1.10.0.
Hide
Permalink
Stephan "Bladed" de Souza added a comment -

Still exists on 1.10.0

Show
Stephan "Bladed" de Souza added a comment - Still exists on 1.10.0
Hide
Permalink
Ralph Schindler added a comment -

I'm looking into this today

Show
Ralph Schindler added a comment - I'm looking into this today
Hide
Permalink
Ralph Schindler added a comment -

The issue is actually not in seek(), seek() seems to be preforming the proper checks. I think the problem you are experiencing is using getRow().

Currently, when you iterate over a rowset, after iteration the internal pointer is set to point beyond the final row of the rowset. While this pauses no general issue with iteration, it will cause an issue when you call getRow($count-1, false), where false is whether or not to seek() BACK to the pointers original position, which of course if invalid.

I'll have to research if having a pointer point beyond the bounds after iteration is really what we want. In the mean time, try to rewind() the rowset after use to avoid any issues with getRow() trying to seek to an invalid position.

Show
Ralph Schindler added a comment - The issue is actually not in seek(), seek() seems to be preforming the proper checks. I think the problem you are experiencing is using getRow(). Currently, when you iterate over a rowset, after iteration the internal pointer is set to point beyond the final row of the rowset. While this pauses no general issue with iteration, it will cause an issue when you call getRow($count-1, false), where false is whether or not to seek() BACK to the pointers original position, which of course if invalid. I'll have to research if having a pointer point beyond the bounds after iteration is really what we want. In the mean time, try to rewind() the rowset after use to avoid any issues with getRow() trying to seek to an invalid position.
Hide
Permalink
Ralph Schindler added a comment -

Adding patch for unit test.

Show
Ralph Schindler added a comment - Adding patch for unit test.
Hide
Permalink
kalyanraj added a comment -

The fix for the illegal index is
if ($seek == true) { $this->seek($key); }

Show
kalyanraj added a comment - The fix for the illegal index is if ($seek == true) { $this->seek($key); }
Hide
Permalink
Stephan "Bladed" de Souza added a comment -

The problem is a simple out of range checking, as proposed on the issue.

If you has a Rowset with 4 rows and you check the last on a while loop will assume:

 
$this->_count = 4 // The last index is 3 (count - 1)
$position = 4 // The now working last index, not count - 1

The script:

 
if ($position < 0 || $position >= $this->_count) {

The result on Issue

 
if (4 < 0 || 4 >= 4) { 
TRUE

Proposed

 
if ($position < 0 || $position > $this->_count) {

The result

 
if (4 < 0 || 4 > 4) { 
FALSE

It NOT throws an Exception and assuming the last pos is 4, and not 3 ( count - 1 ).

And Ralph, do the unit-testing with proposed one, and check! I repeat myself: Its a simple "out of range", the count value is NEVER the last index!

Show
Stephan "Bladed" de Souza added a comment - The problem is a simple out of range checking, as proposed on the issue. If you has a Rowset with 4 rows and you check the last on a while loop will assume: 
 
$this->_count = 4 // The last index is 3 (count - 1)
$position = 4 // The now working last index, not count - 1
The script: 
 
if ($position < 0 || $position >= $this->_count) {
The result on Issue 
 
if (4 < 0 || 4 >= 4) { 
TRUE
Proposed 
 
if ($position < 0 || $position > $this->_count) {
The result 
 
if (4 < 0 || 4 > 4) { 
FALSE
It NOT throws an Exception and assuming the last pos is 4, and not 3 ( count - 1 ). And Ralph, do the unit-testing with proposed one, and check! I repeat myself: Its a simple "out of range", the count value is NEVER the last index!
Hide
Permalink
Ralph Schindler added a comment -

I will agree that calling getRow() after an iteration throwing an exception is a bug. The problem is this: iteration happens, the pointer is left if an invalid state (basically the last element plus 1)- which is the correct behavior in PHP. The problem comes in that getRow() is (wrongly) attempting to leverage seeking and current() to find the desired row. THIS is the wrong thing to do, especially since it will try to restore the seek position after it is done. Since the seek position was invalid to begin off with, it will throw an exception when attempting to restore the seek position to the invalid state.

When you say this:

if ($position < 0 || $position > $this->_count) {

is what should be done, this is wrong. The last valid position is the count - 1, which is why the right hand side is $position >= $this->_count. In other words, if the count is 4, the last valid position is index 3, not 4. This is why we want the exception to be thrown. Put yet another way, the valid range for seek() is 0 to N-1, Not 1 to N.

I have a fix none-the-less. Will apply soon.

-ralph

Show
Ralph Schindler added a comment - I will agree that calling getRow() after an iteration throwing an exception is a bug. The problem is this: iteration happens, the pointer is left if an invalid state (basically the last element plus 1)- which is the correct behavior in PHP. The problem comes in that getRow() is (wrongly) attempting to leverage seeking and current() to find the desired row. THIS is the wrong thing to do, especially since it will try to restore the seek position after it is done. Since the seek position was invalid to begin off with, it will throw an exception when attempting to restore the seek position to the invalid state. When you say this: 
if ($position < 0 || $position > $this->_count) {
is what should be done, this is wrong. The last valid position is the count - 1, which is why the right hand side is $position >= $this->_count. In other words, if the count is 4, the last valid position is index 3, not 4. This is why we want the exception to be thrown. Put yet another way, the valid range for seek() is 0 to N-1, Not 1 to N. I have a fix none-the-less. Will apply soon. -ralph
Hide
Permalink
Ralph Schindler added a comment -

Fixed at r23379 in trunk and at r23380 in release branch 1.11

Show
Ralph Schindler added a comment - Fixed at r23379 in trunk and at r23380 in release branch 1.11

People

Vote (4)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Zend Framework. Try JIRA - bug tracking software for your team.