A Potential Bug

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

A Potential Bug

Dongpeng Xu
Hi, all,

I am using our automatic bug finding tool to scan the source code of
sqlite. The tool is designed to find potential null dereference bug. It
issues warning for the function sqlite3VdbeAllocUnpackedRecord.

SQLITE_PRIVATE UnpackedRecord *sqlite3VdbeAllocUnpackedRecord(
  KeyInfo *pKeyInfo,              /* Description of the record */
  char *pSpace,                   /* Unaligned space available */
  int szSpace,                    /* Size of pSpace[] in bytes */
  char **ppFree                   /* OUT: Caller should free this pointer */
){
  UnpackedRecord *p;              /* Unpacked record to return */
  int nOff;                       /* Increment pSpace by nOff to align it */
  int nByte;                      /* Number of bytes required for *p */

  /* We want to shift the pointer pSpace up such that it is 8-byte aligned.
  ** Thus, we need to calculate a value, nOff, between 0 and 7, to shift
  ** it by.  If pSpace is already 8-byte aligned, nOff should be zero.
  */
  nOff = (8 - (SQLITE_PTR_TO_INT(pSpace) & 7)) & 7;
  nByte = ROUND8(sizeof(UnpackedRecord)) + sizeof(Mem)*(pKeyInfo->nField+1);
  if( nByte>szSpace+nOff ){
    p = (UnpackedRecord *)sqlite3DbMallocRaw(pKeyInfo->db, nByte);
    *ppFree = (char *)p;
    if( !p ) return 0;
  }else{
    p = (UnpackedRecord*)&pSpace[nOff];
    *ppFree = 0;
  }

  p->aMem = (Mem*)&((char*)p)[ROUND8(sizeof(UnpackedRecord))];
  assert( pKeyInfo->aSortOrder!=0 );
  p->pKeyInfo = pKeyInfo;
  p->nField = pKeyInfo->nField + 1;
  return p;
}

The suspicious context is in the function sqlite3VdbeSorterInit, it calls
sqlite3VdbeAllocUnpackedRecord as below:
pSorter->pUnpacked = sqlite3VdbeAllocUnpackedRecord(pCsr->pKeyInfo, 0, 0,
&d);

The second and third parameters are set to zero. However, in
sqlite3VdbeAllocUnpackedRecord, p is set to pSpace + nOff if nByte <=
szSpace + nOff and will be dereferenced later.

I am wondering whether this is a real bug. Is there a concrete execution
path that reach the dereference point? Any comments are welcome. Thanks!

Sincerely,
Dongpeng
_______________________________________________
sqlite-users mailing list
[hidden email]
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: A Potential Bug

David Empson
In this case, sqlite3VdbeAllocUnpackedRecord is called with pSpace = 0 and szSpace = 0.

The calculated value of nOff will also be 0, since pSpace is 0. nByte must be greater than zero, as it is the sum of two positive terms.

Therefore the test "if( nByte>szSpace+nOff )" will be true, and the code path taken will allocate memory.

Not a bug.

On 16/07/2014, at 1:31 pm, Dongpeng Xu <[hidden email]> wrote:

> Hi, all,
>
> I am using our automatic bug finding tool to scan the source code of
> sqlite. The tool is designed to find potential null dereference bug. It
> issues warning for the function sqlite3VdbeAllocUnpackedRecord.
>
> SQLITE_PRIVATE UnpackedRecord *sqlite3VdbeAllocUnpackedRecord(
>  KeyInfo *pKeyInfo,              /* Description of the record */
>  char *pSpace,                   /* Unaligned space available */
>  int szSpace,                    /* Size of pSpace[] in bytes */
>  char **ppFree                   /* OUT: Caller should free this pointer */
> ){
>  UnpackedRecord *p;              /* Unpacked record to return */
>  int nOff;                       /* Increment pSpace by nOff to align it */
>  int nByte;                      /* Number of bytes required for *p */
>
>  /* We want to shift the pointer pSpace up such that it is 8-byte aligned.
>  ** Thus, we need to calculate a value, nOff, between 0 and 7, to shift
>  ** it by.  If pSpace is already 8-byte aligned, nOff should be zero.
>  */
>  nOff = (8 - (SQLITE_PTR_TO_INT(pSpace) & 7)) & 7;
>  nByte = ROUND8(sizeof(UnpackedRecord)) + sizeof(Mem)*(pKeyInfo->nField+1);
>  if( nByte>szSpace+nOff ){
>    p = (UnpackedRecord *)sqlite3DbMallocRaw(pKeyInfo->db, nByte);
>    *ppFree = (char *)p;
>    if( !p ) return 0;
>  }else{
>    p = (UnpackedRecord*)&pSpace[nOff];
>    *ppFree = 0;
>  }
>
>  p->aMem = (Mem*)&((char*)p)[ROUND8(sizeof(UnpackedRecord))];
>  assert( pKeyInfo->aSortOrder!=0 );
>  p->pKeyInfo = pKeyInfo;
>  p->nField = pKeyInfo->nField + 1;
>  return p;
> }
>
> The suspicious context is in the function sqlite3VdbeSorterInit, it calls
> sqlite3VdbeAllocUnpackedRecord as below:
> pSorter->pUnpacked = sqlite3VdbeAllocUnpackedRecord(pCsr->pKeyInfo, 0, 0,
> &d);
>
> The second and third parameters are set to zero. However, in
> sqlite3VdbeAllocUnpackedRecord, p is set to pSpace + nOff if nByte <=
> szSpace + nOff and will be dereferenced later.
>
> I am wondering whether this is a real bug. Is there a concrete execution
> path that reach the dereference point? Any comments are welcome. Thanks!
>
> Sincerely,
> Dongpeng

_______________________________________________
sqlite-users mailing list
[hidden email]
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users