Opened 6 years ago

Closed 5 years ago

#10224 closed bug (no change required)

Changed some dynamic casts to reinterpret casts

Reported by: Ezodev Owned by: anevilyak
Priority: normal Milestone: Unscheduled
Component: Applications/Debugger Version: R1/Development
Keywords: gci2013 Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All


These dynamic casts wern't checked. Reinterpret cast is instant, dynamic cast is slow. If there is no error checking, dynamic cast gives nothing. It will just cause application crash.

Attachments (4)

0001-Changed-unchecked-dynamic-casts-to-reinterpret-casts.patch (4.5 KB ) - added by Ezodev 6 years ago.
0001-Added-nullptr-chcking-after-dynamic-casts.patch (5.0 KB ) - added by Ezodev 6 years ago.
new patch
0002-Dynamic-Cast-error-checking.patch (4.4 KB ) - added by Ezodev 6 years ago.
dynamic-casts-error-checking.patch (4.0 KB ) - added by Ezodev 6 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 by Ezodev, 6 years ago

Has a Patch: set

comment:2 by umccullough, 6 years ago

Per Ezo in IRC, this should resolve CIDs 611227 and 611228 (and possibly others)

comment:3 by bonefish, 6 years ago

The cast operator you're looking for is static_cast, not reinterpret_cast.

The unchecked dynamic_casts are really intended, though. In all cases the actual type of the object is ensured by other means. A mismatch would be a program error and the immediate crash caused by the NULL pointer would be desirable (a static_cast to a wrong type wouldn't necessarily cause an immediate crash). Performance isn't much of a concern in these cases. I don't know, maybe adding an ASSERT would be cleaner. Or alternatively dereferencing the original pointer and casting the reference (causing an exception on type mismatch).

comment:4 by Ezodev, 6 years ago

If speed isn't critical, I've added null checking. It's better than crash few lines later, It will call destructors etc. Error code 2 because it's unused and someone can easily track problem in source. And it will silient off tools to static analysis.

Last edited 6 years ago by Ezodev (previous) (diff)

comment:5 by bonefish, 6 years ago

A call to debugger() would be much better than exit()ing in those cases, ideally with a way to recover afterward, if that doesn't need more than a simple return.

As per our coding style guide there should be a space between if and (.

comment:6 by Ezodev, 6 years ago

I've added calls to debugger(). But I don't understand what you means by way to recover afterward. If this is simply returning from function, I don't know where can I do it without problems.

comment:7 by Ezodev, 6 years ago

Ups. new patch don't compiling, sorry.

Last edited 6 years ago by Ezodev (previous) (diff)

by Ezodev, 6 years ago

comment:8 by Ezodev, 6 years ago

Fixed. Is this solution good?

comment:9 by bonefish, 6 years ago

  • Don't use fprintf(). Just call debugger() in all cases. And return afterward.
  • String literals should not be wrapped like this. Close the literal at the end of the line and start a new on the next line (indent as usual with one additional tab), like:
    printf("this line is "
    	"too long\n");
  • Null pointer checks should be spelled explicitly: foo == NULL.

comment:10 by luroh, 5 years ago

Milestone: R1Unscheduled

Moving Debugger related tickets out of R1 milestone (Prop #14).

comment:11 by anevilyak, 5 years ago

Resolution: no change required
Status: newclosed
Note: See TracTickets for help on using tickets.