Re: [sqlite] Potential corruption bug in 2.8.17. Patch attached.
[EMAIL PROTECTED] wrote:
> >>
> >> --- vdbe.c~2005-12-19 12:42:25.0 -0500
> >> +++ vdbe.c 2006-10-22 16:32:45.0 -0400
> >> @@ -2937,7 +2937,7 @@
> >>if( pOp->p2 & OPFLAG_NCHANGE ) db->nChange++;
> >>if( pOp->p2 & OPFLAG_LASTROWID ) db->lastRowid = pNos->i;
> >>if( pOp->p2 & OPFLAG_CSCHANGE ) db->csChange++;
> >> - if( pC->nextRowidValid && pTos->i>=pC->nextRowid ){
> >> + if( pC->nextRowidValid && pNos->i>=pC->nextRowid ){
> >> pC->nextRowidValid = 0;
> >>}
> >> }
> >>
> >
>
> > The fix checked in was to remove the test altogether and unconditionally set
> > pC->nextRowidValid to 0.
>
> If, as you say, pC->nextRowidValid is always false anyway, wouldn't the
> correct fix be to not even unconditionally set pC->nextRowidValid to 0; just
> delete those two original lines entirely? It sounds like you're now
> unnecessarily setting a variable that's already false to false. (Or did I
> misunderstand your statement?)
>
I could not find a case where pC->nextRowidValid was true. But
neither did I prove it was impossible. Lets just say it was
very unlikely.
Setting pC->nextRowidValid merely invalidates a cache. Invalidating
the cache might make the code a little slower, but it will still get
the correct answer. So it is always safe to set pC->newRowidValid to
0. But we cannot leave pC->nextRowidValid in the true state because
that might leave an invalid value in the cache, resulting in a wrong
answer. So for safety, we always clear the cache here, even though
we have never seen an example where it is necessary. Better safe than
sorry.
--
D. Richard Hipp <[EMAIL PROTECTED]>
-
To unsubscribe, send email to [EMAIL PROTECTED]
-
Re: [sqlite] Potential corruption bug in 2.8.17. Patch attached.
[EMAIL PROTECTED] writes:
> [EMAIL PROTECTED] wrote:
>> This was likely a typo. In its current state, it's accessing uninitialized
>> memory. It looks like it's conceivable that an incorrect nextRowid could be
>> later used if the uninitialized value happens to be a small integer (smaller
>> than pC->nextRowid) and the "valid" flag therefore doesn't get set to false.
>>
>> --- vdbe.c~ 2005-12-19 12:42:25.0 -0500
>> +++ vdbe.c 2006-10-22 16:32:45.0 -0400
>> @@ -2937,7 +2937,7 @@
>>if( pOp->p2 & OPFLAG_NCHANGE ) db->nChange++;
>>if( pOp->p2 & OPFLAG_LASTROWID ) db->lastRowid = pNos->i;
>>if( pOp->p2 & OPFLAG_CSCHANGE ) db->csChange++;
>> - if( pC->nextRowidValid && pTos->i>=pC->nextRowid ){
>> + if( pC->nextRowidValid && pNos->i>=pC->nextRowid ){
>> pC->nextRowidValid = 0;
>>}
>> }
>>
>
> As it happens, pC->nextRowidValid is always false in this
> context, as far as I can tell, so the pTos->i variable is
> never accessed.
That explains why in many years of use, I've never seen an actual corruption
caused by this. Thanks.
> The fix checked in was to remove the test altogether and unconditionally set
> pC->nextRowidValid to 0.
If, as you say, pC->nextRowidValid is always false anyway, wouldn't the
correct fix be to not even unconditionally set pC->nextRowidValid to 0; just
delete those two original lines entirely? It sounds like you're now
unnecessarily setting a variable that's already false to false. (Or did I
misunderstand your statement?)
Derrell
-
To unsubscribe, send email to [EMAIL PROTECTED]
-
Re: [sqlite] Potential corruption bug in 2.8.17. Patch attached.
[EMAIL PROTECTED] wrote:
> This was likely a typo. In its current state, it's accessing uninitialized
> memory. It looks like it's conceivable that an incorrect nextRowid could be
> later used if the uninitialized value happens to be a small integer (smaller
> than pC->nextRowid) and the "valid" flag therefore doesn't get set to false.
>
> --- vdbe.c~ 2005-12-19 12:42:25.0 -0500
> +++ vdbe.c2006-10-22 16:32:45.0 -0400
> @@ -2937,7 +2937,7 @@
>if( pOp->p2 & OPFLAG_NCHANGE ) db->nChange++;
>if( pOp->p2 & OPFLAG_LASTROWID ) db->lastRowid = pNos->i;
>if( pOp->p2 & OPFLAG_CSCHANGE ) db->csChange++;
> - if( pC->nextRowidValid && pTos->i>=pC->nextRowid ){
> + if( pC->nextRowidValid && pNos->i>=pC->nextRowid ){
> pC->nextRowidValid = 0;
>}
> }
>
As it happens, pC->nextRowidValid is always false in this
context, as far as I can tell, so the pTos->i variable is
never accessed. The fix checked in was to remove the test
altogether and unconditionally set pC->nextRowidValid to 0.
Because the code is unreachable, this fix does not require
a new release of SQLite 2.8.
--
D. Richard Hipp <[EMAIL PROTECTED]>
-
To unsubscribe, send email to [EMAIL PROTECTED]
-
[sqlite] Potential corruption bug in 2.8.17. Patch attached.
This was likely a typo. In its current state, it's accessing uninitialized
memory. It looks like it's conceivable that an incorrect nextRowid could be
later used if the uninitialized value happens to be a small integer (smaller
than pC->nextRowid) and the "valid" flag therefore doesn't get set to false.
--- vdbe.c~ 2005-12-19 12:42:25.0 -0500
+++ vdbe.c 2006-10-22 16:32:45.0 -0400
@@ -2937,7 +2937,7 @@
if( pOp->p2 & OPFLAG_NCHANGE ) db->nChange++;
if( pOp->p2 & OPFLAG_LASTROWID ) db->lastRowid = pNos->i;
if( pOp->p2 & OPFLAG_CSCHANGE ) db->csChange++;
- if( pC->nextRowidValid && pTos->i>=pC->nextRowid ){
+ if( pC->nextRowidValid && pNos->i>=pC->nextRowid ){
pC->nextRowidValid = 0;
}
}
Cheers,
Derrell
-
To unsubscribe, send email to [EMAIL PROTECTED]
-
