Opened 4 years ago

Closed 4 years ago

#15304 closed defect (invalid)

Checking wxWidgets by Klocwork: Null pointer may be dereferenced in html\htmlwin.cpp: 429, 435

Reported by: Mithrandir Owned by:
Priority: normal Milestone: 2.9.5
Component: wxHtml Version: 2.8.12
Keywords: Klocwork Null pointer dereferenced htmlwin.cpp Cc:
Blocked By: Blocking:
Patch: no

Description

Several real and potential issues were found while checking wxWidgets by Klocwork (Source Code Analysis Tools for Software Security) on Windows and Linux. The latest stable wxWidgets version 2.8.12 has been checked, but all issues has been existing in the current trunk too.

Null pointer 'nodeL.operator->()' that comes from line 410 may be dereferenced at line 429.

Null pointer 'nodeG.operator->()' that comes from line 410 may be dereferenced at line 435.

Issue Name: Null pointer may be dereferenced 
Location: html\htmlwin.cpp: 429, 435

Source code:

423          while (nodeL
nodeG)
424          {
425              prL = (nodeL) ? nodeL->GetData()->GetPriority() : -1;
426              prG = (nodeG) ? nodeG->GetData()->GetPriority() : -1;
427              if (prL > prG)
428              {
429                  if (nodeL->GetData()->IsEnabled())
430                      newsrc = nodeL->GetData()->Process(newsrc);
431                  nodeL = nodeL->GetNext();
432              }
433              else prL <= prG
434              {
435                  if (nodeG->GetData()->IsEnabled())
436                      newsrc = nodeG->GetData()->Process(newsrc);
437                  nodeG = nodeG->GetNext();
438              }
439          }
440      }

Change History (3)

comment:1 Changed 4 years ago by vadz

  • Resolution set to invalid
  • Status changed from new to closed

I don't know this code but assuming GetPriority() always returns something positive, I don't see any problem here because prL > prG implies nodeL != NULL.

Please explain if you still think this is a real problem, thanks.

comment:2 Changed 4 years ago by Mithrandir

  • Resolution invalid deleted
  • Status changed from closed to reopened

GetPriority() returns int (see declaration), so it may return value <= -1.

43      Return priority value of this processor. The higher, the sooner
44     
is the processor applied to the text.
45      virtual int GetPriority() const { return wxHTML_PRIORITY_DONTCARE; }

There is no problem if GetPriority() always returns something positive.

In my opinion it is not good practice to check a flag and external data in one condition. It is not clear and add hidden dependency...

comment:3 Changed 4 years ago by vadz

  • Resolution set to invalid
  • Status changed from reopened to closed

Yes, in principle it might return a negative value. But in this code, AFAICS, it only returns positive values.

If you'd like to rewrite this code in a more clear way, you may try to do it and submit a patch with your changes but this is hardly a critical thing to do and there is a huge difference between "not clear code" and "a potential crash".

Note: See TracTickets for help on using tickets.