Re: [fossil-dev] Bug - Fossil source "printf.c"

[Richard Hipp]

On 5/21/17, David Simmons  wrote:
> Location: "fossil\src\printf.c"
> Section(s): (line 385)
>  /* Limit the precision to prevent overflowing buf[] during
> conversion */
>  if( precision > (etBUFSIZE-40) && (infop->flags & FLAG_STRING)==0 ){
>precision = etBUFSIZE-40;
>  }
>
> Issue: missing parens to ensure correct ordering of expressing. Lacking
> parens, it sets precision to "etBUFSIZE-40" ~ (500-40) => 460
> Impact: inefficient behavior, possible output errors.

What compiler are you using that thinks that the ">" operator has
higher precedence than the "-" operator?

And I don't quite understand what the second complaint (below) is
about.  Could you explain the problem again - perhaps with an example
that shows the failure?

>
> Location: "fossil\src\printf.c"
> Section(s): (line 200 ... line 645)
>  static int StrNLen32(const char *z, int N){
>  ...elided...
>case etSTRINGID:
>case etSTRING:
>case etDYNSTRING: {
>  int limit = flag_alternateform ? va_arg(ap,int) : -1;
>  bufpt = va_arg(ap,char*);
>  if(bufpt == 0){
>bufpt = "";
>  }else if( xtype==etDYNSTRING ){
>zExtra = bufpt;
>  }else if( xtype==etSTRINGID ){
>precision = hashDigits(flag_altform2);
>  }
>  // printf rules say: (fixes from DSIM)
>  //  "width" is a minimum for the output of this param
>  //  "precision" is a maximum or exact size of this param
>  if(precision >= 0 && xtype != etSTRINGID) {
>limit = length = precision;
>int uLen32 = StrNLen32(bufpt, limit);
>// DSim: assert(uLen32 == length);
>  }
>  else {
>if(limit < 0) {
>  if(precision > 0)
>limit = precision;
>  else if(width)
>limit = width;
>}
>else if(limit > precision) {
>  limit = precision;
>}
>else if(limit > width) {
>  limit = width;
>}
>// DSIM: This code has a bug where it assumes NULL '\0'
> terminated bufpt
>// However, if *precision* is provided and > 0 and limit is
> -1 then
>// precision defines maximum number of characters in bufpt.
> Without
>// that rule, and bufpt not being '\0' null terminated, this code
>// BLOWS UP randomly since bufpt memory can be random beyond
> bufpt[precision].
>length = StrNLen32(bufpt, limit); // <== SOURCE OF BUG (GOES
> BOOM RARELY, BUT RANDOMLY!)
>  }
>  break;
>}
>
> Issue: code does not insure reading past end of z, due to calling code
> in '%...s' miscalculation of default value for N.
> Impact: intermittent app-crashes
> ___
> fossil-dev mailing list
> fossil-dev@mailinglists.sqlite.org
> http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/fossil-dev
>


-- 
D. Richard Hipp
d...@sqlite.org
___
fossil-dev mailing list
fossil-dev@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/fossil-dev

Re: [fossil-dev] Bug - Fossil source "printf.c"

[Stephan Beal]

Just to play Devil's Advocate...

On Sun, May 21, 2017 at 11:00 PM, David Simmons 
wrote:

>   // DSIM: This code has a bug where it assumes NULL '\0'
> terminated bufpt
>
...

and now it's got TWO bugs: C++-style comments in C89 code ;).

-- 
- stephan beal
http://wanderinghorse.net/home/stephan/
"Freedom is sloppy. But since tyranny's the only guaranteed byproduct of
those who insist on a perfect world, freedom will have to do." -- Bigby Wolf
___
fossil-dev mailing list
fossil-dev@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/fossil-dev

Re: [fossil-dev] Bug - Fossil source "printf.c"

[David Simmons]

Addendum:

Also note that the "signed/unsigned" compare possibility with etBUFSIZE 
being "unsigned" in "clang" makes this error worse, since expression 
needs to ensure precision not promoted to "unsigned".


-- DSim

David Simmons 
Sunday, May 21, 2017 2:00 PM
Location: "fossil\src\printf.c"
Section(s): (line 385)
/* Limit the precision to prevent overflowing buf[] during 
conversion */
if( precision > (int)(etBUFSIZE-40) && (infop->flags & 
FLAG_STRING)==0 ){

  precision = etBUFSIZE-40;
}

Issue: missing parens to ensure correct ordering of expression. 
Lacking parens, it sets precision to "etBUFSIZE-40" ~ (500-40) => 460

Impact: inefficient behavior, possible output errors.

Location: "fossil\src\printf.c"
Section(s): (line 200 ... line 645)
static int StrNLen32(const char *z, int N){
...elided...
  case etSTRINGID:
  case etSTRING:
  case etDYNSTRING: {
int limit = flag_alternateform ? va_arg(ap,int) : -1;
bufpt = va_arg(ap,char*);
if(bufpt == 0){
  bufpt = "";
}else if( xtype==etDYNSTRING ){
  zExtra = bufpt;
}else if( xtype==etSTRINGID ){
  precision = hashDigits(flag_altform2);
}
// printf rules say: (fixes from DSIM)
//  "width" is a minimum for the output of this param
//  "precision" is a maximum or exact size of this param
if(precision >= 0 && xtype != etSTRINGID) {
  limit = length = precision;
  int uLen32 = StrNLen32(bufpt, limit);
  // DSim: assert(uLen32 == length);
}
else {
  if(limit < 0) {
if(precision > 0)
  limit = precision;
else if(width)
  limit = width;
  }
  else if(limit > precision) {
limit = precision;
  }
  else if(limit > width) {
limit = width;
  }
  // DSIM: This code has a bug where it assumes NULL '\0' 
terminated bufpt
  // However, if *precision* is provided and > 0 and limit is 
-1 then
  // precision defines maximum number of characters in bufpt. 
Without
  // that rule, and bufpt not being '\0' null terminated, this 
code
  // BLOWS UP randomly since bufpt memory can be random beyond 
bufpt[precision].
  length = StrNLen32(bufpt, limit); // <== SOURCE OF BUG (GOES 
BOOM RARELY, BUT RANDOMLY!)

}
break;
  }

Issue: code does not insure reading past end of z, due to calling code 
in '%...s' miscalculation of default value for N.

Impact: intermittent app-crashes


___
fossil-dev mailing list
fossil-dev@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/fossil-dev

[fossil-dev] Bug - Fossil source "printf.c"

[David Simmons]

Location: "fossil\src\printf.c"
Section(s): (line 385)
/* Limit the precision to prevent overflowing buf[] during 
conversion */

if( precision > (etBUFSIZE-40) && (infop->flags & FLAG_STRING)==0 ){
  precision = etBUFSIZE-40;
}

Issue: missing parens to ensure correct ordering of expressing. Lacking 
parens, it sets precision to "etBUFSIZE-40" ~ (500-40) => 460

Impact: inefficient behavior, possible output errors.

Location: "fossil\src\printf.c"
Section(s): (line 200 ... line 645)
static int StrNLen32(const char *z, int N){
...elided...
  case etSTRINGID:
  case etSTRING:
  case etDYNSTRING: {
int limit = flag_alternateform ? va_arg(ap,int) : -1;
bufpt = va_arg(ap,char*);
if(bufpt == 0){
  bufpt = "";
}else if( xtype==etDYNSTRING ){
  zExtra = bufpt;
}else if( xtype==etSTRINGID ){
  precision = hashDigits(flag_altform2);
}
// printf rules say: (fixes from DSIM)
//  "width" is a minimum for the output of this param
//  "precision" is a maximum or exact size of this param
if(precision >= 0 && xtype != etSTRINGID) {
  limit = length = precision;
  int uLen32 = StrNLen32(bufpt, limit);
  // DSim: assert(uLen32 == length);
}
else {
  if(limit < 0) {
if(precision > 0)
  limit = precision;
else if(width)
  limit = width;
  }
  else if(limit > precision) {
limit = precision;
  }
  else if(limit > width) {
limit = width;
  }
  // DSIM: This code has a bug where it assumes NULL '\0' 
terminated bufpt
  // However, if *precision* is provided and > 0 and limit is 
-1 then
  // precision defines maximum number of characters in bufpt. 
Without

  // that rule, and bufpt not being '\0' null terminated, this code
  // BLOWS UP randomly since bufpt memory can be random beyond 
bufpt[precision].
  length = StrNLen32(bufpt, limit); // <== SOURCE OF BUG (GOES 
BOOM RARELY, BUT RANDOMLY!)

}
break;
  }

Issue: code does not insure reading past end of z, due to calling code 
in '%...s' miscalculation of default value for N.

Impact: intermittent app-crashes
___
fossil-dev mailing list
fossil-dev@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/fossil-dev