Possible bad error checking in ExceptionHandler::WriteMinidumpForChild()

RESOLVED WORKSFORME

Status

()

RESOLVED WORKSFORME
7 years ago
5 years ago

People

(Reporter: Dolske, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [pvs-studio][good first bug][lang=c++])

(Reporter)

Description

7 years ago
From http://www.viva64.com/en/a/0078/

Example 10. Dumping error

bool ExceptionHandler::WriteMinidumpForChild(...)
{
  ...
  DWORD last_suspend_cnt = -1;
  ...
  // this thread may have died already, so not opening
  // the handle is a non-fatal error
  if (NULL != child_thread_handle) {
    if (0 <= (last_suspend_cnt =
                SuspendThread(child_thread_handle))) {
  ...
}

PVS-Studio diagnostic message: V547 Expression is always true. Unsigned type value is always >= 0. exception_handler.cc 846

This is another example in the error handler. The result returned by the SuspendThread function is processed incorrectly here. The last_suspend_cnt variable has the DWORD type and therefore is always greater or equal to 0.
(Reporter)

Updated

7 years ago
Blocks: 710966
Whiteboard: [pvs-studio] → [pvs-studio][good first bug][lang=c++]
This is this code right here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.cc#846

It probably just needs to be changed to
if ((DWORD)-1 != (last_suspend_cnt = SuspendThread(...))) {

The documentation for SuspendThread says it returns (DWORD)-1 on failure:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms686345%28v=vs.85%29.aspx
Justin, do you think this is still a valid issue? I'm going through old "good first bug" to verify if they are still valid or something is changed in the meanwhile.
Flags: needinfo?(dolske)

Comment 3

5 years ago
Looks like we imported newer code since this was filed, so the problem doesn't exist any longer.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(dolske)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.