Added/Fixed null pointer checks #10591
Conversation
Rubonnek
force-pushed
the
possible-null-ptr-dereference
branch
2 times, most recently
from
1b66d25 to
25250dd
Compare
editor/editor_node.cpp
Outdated
| @@ -1291,7 +1291,7 @@ void EditorNode::_dialog_action(String p_file) { | |||
| uint32_t current = editor_history.get_current(); | |||
| Object *current_obj = current > 0 ? ObjectDB::get_instance(current) : NULL; | |||
|
|
|||
| ERR_FAIL_COND(!current_obj->cast_to<Resource>()) | |||
There was a problem hiding this comment.
This doesn't seem right, plus it would conflict with #10581.
Basically, ->cast_to was assumed to run on null pointers as well, and it would return null if (1) this == NULL, or (2) this couldn't be casted to the specified type.
There was a problem hiding this comment.
Which is precisely what #10581 fixes :)
editor/editor_settings.cpp
Outdated
| file->reopen(p_path.plus_file((String)keys[i]), FileAccess::WRITE); | ||
| ERR_FAIL_COND(!file); | ||
| Error err = file->reopen(p_path.plus_file((String)keys[i]), FileAccess::WRITE); | ||
| ERR_FAIL_COND(!err); |
There was a problem hiding this comment.
I think you should use err != OK (or Error::OK, if needed). This way, the error message would be clearer.
There was a problem hiding this comment.
I prefer !err, but yeah, err != OK is clearer. I'll do that.
drivers/gles3/rasterizer_gles3.cpp
Outdated
| @@ -247,9 +247,6 @@ void RasterizerGLES3::set_current_render_target(RID p_render_target) { | |||
|
|
|||
| if (p_render_target.is_valid()) { | |||
| RasterizerStorageGLES3::RenderTarget *rt = storage->render_target_owner.getornull(p_render_target); | |||
| if (!rt) { | |||
| storage->frame.current_rt = NULL; | |||
| } | |||
| ERR_FAIL_COND(!rt); | |||
| storage->frame.current_rt = rt; | |||
There was a problem hiding this comment.
I'm not sure if the new behaviour would be correct, but to get the old behaviour, you should move this line above ERR_FAIL_COND.
storage->frame.current_rt = rt;
ERR_FAIL_COND(!rt);There was a problem hiding this comment.
That's right. I'll fix that.
|
I think it's better to wait until #10581 is finished and re-run cppcheck then. |
|
@RandomShaper Sounds good. I will do that then. |
| @@ -570,6 +570,9 @@ CollisionShape2DEditor::CollisionShape2DEditor(EditorNode *p_editor) { | |||
|
|
|||
| void CollisionShape2DEditorPlugin::edit(Object *p_obj) { | |||
|
|
|||
| if(!p_obj) | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
I keep getting these. I'll install the commit hooks :p
There was a problem hiding this comment.
CollisionShape2DEditor::edit() is null safe. This check is no longer required after #10581 either
There was a problem hiding this comment.
It is not. Like I mentioned in the first message:
Starting to edit a collision shape and clicking on a different node of the scene tree will trigger a segmentation fault.
There was a problem hiding this comment.
It might be after #10581, I haven't checked it yet.
editor/editor_node.cpp
Outdated
| @@ -2308,7 +2308,7 @@ void EditorNode::_menu_option_confirm(int p_option, bool p_confirmed) { | |||
| uint32_t current = editor_history.get_current(); | |||
| Object *current_obj = current > 0 ? ObjectDB::get_instance(current) : NULL; | |||
|
|
|||
| ERR_FAIL_COND(!current_obj->cast_to<Resource>()) | |||
| ERR_FAIL_COND(!current_obj) | |||
There was a problem hiding this comment.
The checks in this block are not equivalent to the old code. This code only fails if current_obj is Null, while the previous code would also fail if current_obj couldn't cast to a Resource. Any crashes this used to produce will be fixed with #10581 now.
There was a problem hiding this comment.
Thanks for pointing it out. I'm aware of that. My point on changing these was simply to avoid segmentation faults. I'm waiting for #10581 to get merged to rescan with cppcheck.
|
@hpvb Thanks for letting me know. I'm on it. |
Rubonnek
force-pushed
the
possible-null-ptr-dereference
branch
3 times, most recently
from
8cdd3d9 to
9b1b1be
Compare
|
After rescanning the master branch again, the following messages showed up and These showed up again but like I mentioned in the first post, these seem to be These messages are different to null pointer dereferencing and were not fixed And this last message is a false positive that might show due to |
Rubonnek
force-pushed
the
possible-null-ptr-dereference
branch
4 times, most recently
from
82c1849 to
75ceb82
Compare
|
This pull request now also fixes #10638 |
| @@ -3766,7 +3766,7 @@ bool CanvasItemEditorViewport::_cyclical_dependency_exists(const String &p_targe | |||
|
|
|||
| void CanvasItemEditorViewport::_create_nodes(Node *parent, Node *child, String &path, const Point2 &p_point) { | |||
| child->set_name(path.get_file().get_basename()); | |||
| Ref<Texture> texture = Object::cast_to<Texture>(Ref<Texture>(ResourceCache::get(path)).ptr()); | |||
| Ref<Texture> texture = Ref<Texture>(Object::cast_to<Texture>(ResourceCache::get(path))); | |||
There was a problem hiding this comment.
Thanks for catching this! I'll go through the diff again and make sure I didn't make this mistake elsewhere (I apparently saw Ref and decided I needed to dereference, whoops)
There was a problem hiding this comment.
Anytime! You did fix a lot of issues in your pull request.
For what it's worth, I did a quick egrep to catch another line like that:
egrep -R "Object::cast_to<.*>\(.*Ref"
but nothing showed up. I think the rest is good.
| @@ -215,9 +215,6 @@ void TextEdit::Text::_update_line_cache(int p_line) const { | |||
|
|
|||
| const Map<int, TextEdit::Text::ColorRegionInfo> &TextEdit::Text::get_color_region_info(int p_line) { | |||
|
|
|||
| Map<int, ColorRegionInfo> *cri = NULL; | |||
| ERR_FAIL_INDEX_V(p_line, text.size(), *cri); //enjoy your crash | |||
There was a problem hiding this comment.
I'm not sure if just removing the debugging warning here is a good idea. In release mode this crash already doesn't exist. Maybe find out what's at the other end of this and return a value that won't crash?
There was a problem hiding this comment.
Or maybe make it a fatal error rather than a warning.
There was a problem hiding this comment.
I think that by removing the explicit handling, the container implementation will crash (on purpose) on index-out-of-bounds.
There was a problem hiding this comment.
Wouldn't it be better to crash here though, for debugging purposes?
There was a problem hiding this comment.
I don't think it matters whether it crashes here or not. The backtrace will still point us to the right line.
You can try putting this in, say, the main function.
String text;
text[20]; // crashes hereThis is the gdb message:
Program received signal SIGILL, Illegal instruction.
0x00005555556e7d49 in Vector<wchar_t>::operator[] (p_index=20, this=0x7fffffffdae8) at core/vector.h:137
137 CRASH_BAD_INDEX(p_index, size());
And the backtrace you will get:
(gdb) bt
#0 0x00005555556e7d49 in Vector<wchar_t>::operator[] (p_index=20, this=0x7fffffffdae8) at core/vector.h:137
#1 main (argc=<optimized out>, argv=<optimized out>) at platform/x11/godot_x11.cpp:48
It's obvious what's happening just from that, which is why I think those two lines are unnecessary.
There was a problem hiding this comment.
I think the idea is that for the end user, it's better to have the error message induced by ERR_FAIL_INDEX_V before a segfault that they won't know how to debug. This way they have something meaningful to report to us and not just a "it crashed". But I have no strong opinion either way.
There was a problem hiding this comment.
I'll check how Godot behaves when those lines get executed in a Clang build then and see if I can get both builds to behave the same way while keeping the messages visible.
There was a problem hiding this comment.
Changing the for loop to:
for (int i = -2000; i < cursor.line_ofs + 2000; i++) {to trigger ERR_FAIL_INDEX_V crashes both builds.
But passing a static map keeps both builds from crashing:
static Map<int, ColorRegionInfo> cri;
ERR_FAIL_INDEX_V(p_line, text.size(), cri); //no more crashes on either builds
platform/x11/os_x11.cpp
Outdated
| } | ||
|
|
||
| // Try to support IME if detectable auto-repeat is supported |
There was a problem hiding this comment.
Actually that comment was at the right place, the detection of auto-repeat is that xkb_dar bool.
There was a problem hiding this comment.
Completely missed it when I was copy-pasting. I'll fix it. Thanks.
platform/x11/power_x11.cpp
Outdated
| @@ -58,6 +58,7 @@ Adapted from corresponding SDL 2.0 code. | |||
| #include <stdio.h> | |||
| #include <unistd.h> | |||
|
|
|||
| #include <core/error_macros.h> | |||
There was a problem hiding this comment.
Godot includes should use double quotes, <> are for system headers. So it should be:
#include "power_x11.h"
#include "core/error_macros.h"
#include <dirent.h>
#include <fcntl.h>
#include <stdio.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
There was a problem hiding this comment.
Wasn't aware of this standard. I'll fix it then.
Rubonnek
force-pushed
the
possible-null-ptr-dereference
branch
from
75ceb82 to
2b59660
Compare
Rubonnek
force-pushed
the
possible-null-ptr-dereference
branch
from
2b59660 to
7a07895
Compare
This pull request mitigates some
cppcheckmessages related to null pointerdereferencing. Hopefully this will cover most of the segfaults that could occur when compiling with GCC 7.1.1:
Starting to edit a collision shape and clicking on a different node of the scene tree will trigger a segmentation fault. A fix was applied and mitigates the following message:
The following messages were also mitigates which seemed like bugs or unneeded code:
As far as I can tell, all of the following possible null dereferences seem to have already been
accounted for previously and I did not provide any fixes for these. I'm
including their relevant code blocks in any case for easier inspection, but I don't mind mitigating these messages if required:
cppcheck message:
relevant codeblock:
fnodewas already accounted for in line 239godot/drivers/gles3/shader_compiler_gles3.cpp
Lines 230 to 244 in b4ad899
cppcheck message:
relevant codeblock:
pwas accounted for in line 156godot/editor/editor_dir_dialog.cpp
Lines 148 to 157 in b4ad899
cppcheck message:
relevant codeblock:
ftmpwas accounted for in the firstifstatement belowgodot/editor/editor_export.cpp
Lines 660 to 675 in b4ad899
cppcheck message:
relevant codeblock:
dockwas accounted for in line 4657godot/editor/editor_node.cpp
Lines 4646 to 4661 in b4ad899
cppcheck message:
relevant codeblocks:
I don't think we need a fix for this --
to_nodeonly comes from twodifferent sources:
to_nodewas already accounted for in line 1751 as thenodevariable:godot/editor/scene_tree_dock.cpp
Lines 1748 to 1756 in b4ad899
to_nodewas already accounted for in line 1783:godot/editor/scene_tree_dock.cpp
Lines 1767 to 1790 in b4ad899
cppcheckpoints to:godot/editor/scene_tree_dock.cpp
Lines 1683 to 1696 in b4ad899