Closed Bug 173557 Opened 20 years ago Closed 19 years ago

flawfinder warnings in inspector

Categories

(Other Applications :: DOM Inspector, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: morse, Assigned: caillon)

References

()

Details

Attachments

(1 file, 1 obsolete file)

I run flawfinder (http://www.dwheeler.com/flawfinder) on Mozilla 1.0.1 branch.

flawfinder found 1 warnings in inspector code (1494). Go through
that list and for each warning:

* If it is false positive, comment here why it is not an issue
* If it is a real issue, make patch for it here and let's get them checked in

In addition to checking the branch, also check the trunk.

1494) extensions/inspector/base/src/inBitmap.cpp:112 [2] (buffer) sprintf: does 
not check for buffer overflows. Use snprintf or vsnprintf. Risk is low because 
the source has a constant maximum length.
Blocks: 148251
iirc this file is a clone of an imagelib file.
Assignee: sgehani → hewitt
Component: XP Apps → DOM Inspector
QA Contact: paw → timeless
This code needs to be fixed. First of all, there is a new/delete pair when we
could use the local stack. Also John Keiser seemed to say that we have a better
way to do write-to-char-then-copy-to-nsString. Finally the buffer we allocate
for our string is of size 7, and we sprintf 7 characters into it - so where does
the null character go?
nsPrintfCString is the preferred way to do sprintfs into strings:
#include "nsPrintfCString.h"
nsPrintfCString str("%d", myInt);
interesting, but the final destination is a PRUnichar* not a char* (see blame url).

nsTextFormatter is a much better fit.
the following is my proposed (untested) code
insert @ 40: #include "nsTextFormatter.h"

replace @ 111-123:
*_retval = nsTextFormatter::smprintf("#%2X%2X%2X", r, g, b);
if (!*_retval)
  return NS_ERROR_OUT_OF_MEMORY;
// 0-pad hex values
for (PRUintn i = 1; i <= 6; ++i)
if (_retval[i] == ' ')
  _retval[i] = '0';
Attached patch untested (obsolete) — Splinter Review
Comment on attachment 103211 [details] [diff] [review]
untested

>Index: mozilla/extensions/inspector/base/src/inBitmap.cpp
>===================================================================
>-  if (aX < 0 || aX > mWidth || aY < 0 || aY > mHeight)
>+  if (aX < 0 || aX >= mWidth || aY < 0 || aY >= mHeight)

I don't think this belongs into this patch.

>-  // sprintf won't 0-pad my hex values, so I have to space-pad it
>-  // and then replace space characters with zero characters

>+  // 0-pad hex values
>+  for (PRUintn i = 1; i <= 6; ++i)
>+  if (_retval[i] == ' ')
>+    _retval[i] = '0';

If nsTextFormatter 0-pads we obviously won't need this, but someone needs to
test this first. If we need to 0-pad: the old comment is more clear so it
should stay (just change sprintf with the correct name), also don't change the
for-line. The new if-clause is more readable than the original, so I like it.

Once we get some testing I can give r/sr, given the above changes.
Timeless said the top part really is part of the patch; the allocated mBits size
does not seem to match this code here.

Rather than change this function here I'd like to know if we should simply
allocate more in Init().

Also we need to test if |new| failed in Init(), and if so, return an error (and
the callers should check for that too).
Christopher, this is pretty serious, could you finish the work here?
We really should get this fixed for 1.4b.
Assignee: hewitt → caillon
Flags: blocking1.4b?
OS: Windows NT → All
Hardware: PC → All
I'm really sorry, but there's not one whit of code I can do on this one.  I hack
via Patch Maker (XUL+JS+XBL), and I don't know C++ at all.

caillon is somewhat inactive at this time; if you really want this fixed before
1.4b, you might want to try someone else.
Attachment #103211 - Attachment is obsolete: true
Attachment #121248 - Flags: superreview?(bzbarsky)
Attachment #121248 - Flags: review?(caillon)
Comment on attachment 121248 [details] [diff] [review]
103211 compiling, win caller uncompiled

> NS_IMETHODIMP
> inBitmap::GetPixelHex(PRUint32 aX, PRUint32 aY, PRUnichar **_retval)
> {
>-  if (aX < 0 || aX > mWidth || aY < 0 || aY > mHeight)
>+  if (aX < 0 || aX >= mWidth || aY < 0 || aY >= mHeight)
>     return NS_ERROR_FAILURE;

We should maybe warn here if mBits is null in case anyone decides in the future
to not check Init()'s rv.

>   PRUint8* c = mBits + ((aX+(mWidth*aY))*3);
>   PRUint8 b = c[0];
>   PRUint8 g = c[1];
>   PRUint8 r = c[2];

...

>-  // sprintf won't 0-pad my hex values, so I have to space-pad it
>-  // and then replace space characters with zero characters

I kind of agree with heikki that this comment is a bit clearer than your
replacement (if you update the function name).	But I'm not attached to it
either way.  r=caillon.
Attachment #121248 - Flags: review?(caillon) → review+
Comment on attachment 121248 [details] [diff] [review]
103211 compiling, win caller uncompiled

>     mBits = new PRUint8[aWidth*aHeight*3];
>+  } else {
>+    return NS_ERROR_UNEXPECTED;

NS_ERROR before the return, for good measure.

sr=me with that.
Attachment #121248 - Flags: superreview?(bzbarsky) → superreview+
...Oh and by the way, this is fixed.  :-)
Really this time (oops)!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Flags: blocking1.4b?
Product: Core → Other Applications
QA Contact: timeless → dom-inspector
You need to log in before you can comment on or make changes to this bug.