Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added/Fixed null pointer checks #10591

Merged
merged 1 commit into from
Aug 27, 2017
Merged

Added/Fixed null pointer checks #10591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions drivers/gles3/rasterizer_gles3.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,11 +247,8 @@ 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;
ERR_FAIL_COND(!rt);
storage->frame.clear_request = false;

glViewport(0, 0, rt->width, rt->height);
Expand Down
2 changes: 1 addition & 1 deletion editor/animation_editor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1646,7 +1646,7 @@ PropertyInfo AnimationKeyEditor::_find_hint_for_track(int p_idx, NodePath &r_bas
List<PropertyInfo> pinfo;
if (res.is_valid())
res->get_property_list(&pinfo);
else
else if (node)
node->get_property_list(&pinfo);

for (List<PropertyInfo>::Element *E = pinfo.front(); E; E = E->next()) {
Expand Down
4 changes: 2 additions & 2 deletions editor/editor_settings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,8 @@ static void _create_script_templates(const String &p_path) {
dir->change_dir(p_path);
for (int i = 0; i < keys.size(); i++) {
if (!dir->file_exists(keys[i])) {
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 != OK);
file->store_string(templates[keys[i]]);
file->close();
}
Expand Down
2 changes: 1 addition & 1 deletion editor/plugins/canvas_item_editor_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@Rubonnek Rubonnek Aug 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Size2 texture_size = texture->get_size();

editor_data->get_undo_redo().add_do_method(parent, "add_child", child);
Expand Down
6 changes: 3 additions & 3 deletions modules/visual_script/visual_script_func_nodes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -822,7 +822,7 @@ class VisualScriptNodeInstanceFunctionCall : public VisualScriptNodeInstance {
}

Node *another = node->get_node(node_path);
if (!node) {
if (!another) {
r_error.error = Variant::CallError::CALL_ERROR_INVALID_METHOD;
r_error_str = "Path does not lead Node!";
return 0;
Expand Down Expand Up @@ -1596,7 +1596,7 @@ class VisualScriptNodeInstancePropertySet : public VisualScriptNodeInstance {
}

Node *another = node->get_node(node_path);
if (!node) {
if (!another) {
r_error.error = Variant::CallError::CALL_ERROR_INVALID_METHOD;
r_error_str = "Path does not lead Node!";
return 0;
Expand Down Expand Up @@ -2241,7 +2241,7 @@ class VisualScriptNodeInstancePropertyGet : public VisualScriptNodeInstance {
}

Node *another = node->get_node(node_path);
if (!node) {
if (!another) {
r_error.error = Variant::CallError::CALL_ERROR_INVALID_METHOD;
r_error_str = RTR("Path does not lead Node!");
return 0;
Expand Down
2 changes: 1 addition & 1 deletion modules/visual_script/visual_script_nodes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2087,7 +2087,7 @@ class VisualScriptNodeInstanceSceneNode : public VisualScriptNodeInstance {
}

Node *another = node->get_node(path);
if (!node) {
if (!another) {
r_error.error = Variant::CallError::CALL_ERROR_INVALID_METHOD;
r_error_str = "Path does not lead Node!";
return 0;
Expand Down
2 changes: 1 addition & 1 deletion modules/visual_script/visual_script_yield_nodes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ class VisualScriptNodeInstanceYieldSignal : public VisualScriptNodeInstance {
}

Node *another = node->get_node(node_path);
if (!node) {
if (!another) {
r_error.error = Variant::CallError::CALL_ERROR_INVALID_METHOD;
r_error_str = "Path does not lead Node!";
return 0;
Expand Down
14 changes: 5 additions & 9 deletions platform/x11/os_x11.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,33 +116,29 @@ void OS_X11::initialize(const VideoMode &p_desired, int p_video_driver, int p_au
/** XLIB INITIALIZATION **/
x11_display = XOpenDisplay(NULL);

char *modifiers = NULL;
Bool xkb_dar = False;
if (x11_display) {
XAutoRepeatOn(x11_display);
xkb_dar = XkbSetDetectableAutoRepeat(x11_display, True, NULL);
}

char *modifiers = NULL;

// Try to support IME if detectable auto-repeat is supported

if (xkb_dar == True) {
// Try to support IME if detectable auto-repeat is supported
if (xkb_dar == True) {

// Xutf8LookupString will be used later instead of XmbLookupString before
// the multibyte sequences can be converted to unicode string.

#ifdef X_HAVE_UTF8_STRING
modifiers = XSetLocaleModifiers("");
modifiers = XSetLocaleModifiers("");
#endif
}
}

if (modifiers == NULL) {
if (is_stdout_verbose()) {
WARN_PRINT("IME is disabled");
}
modifiers = XSetLocaleModifiers("@im=none");
}
if (modifiers == NULL) {
WARN_PRINT("Error setting locale modifiers");
}

Expand Down
13 changes: 7 additions & 6 deletions platform/x11/power_x11.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ Adapted from corresponding SDL 2.0 code.
#include <stdio.h>
#include <unistd.h>

#include "core/error_macros.h"
#include <dirent.h>
#include <fcntl.h>
#include <sys/stat.h>
Expand Down Expand Up @@ -254,9 +255,9 @@ bool PowerX11::GetPowerInfo_Linux_proc_acpi() {
this->power_state = POWERSTATE_UNKNOWN;

dirp->change_dir(proc_acpi_battery_path);
dirp->list_dir_begin();
Error err = dirp->list_dir_begin();

if (dirp == NULL) {
if (err != OK) {
return false; /* can't use this interface. */
} else {
node = dirp->get_next();
Expand All @@ -268,8 +269,8 @@ bool PowerX11::GetPowerInfo_Linux_proc_acpi() {
}

dirp->change_dir(proc_acpi_ac_adapter_path);
dirp->list_dir_begin();
if (dirp == NULL) {
err = dirp->list_dir_begin();
if (err != OK) {
return false; /* can't use this interface. */
} else {
node = dirp->get_next();
Expand Down Expand Up @@ -438,9 +439,9 @@ bool PowerX11::GetPowerInfo_Linux_sys_class_power_supply(/*PowerState *state, in

DirAccess *dirp = DirAccess::create(DirAccess::ACCESS_FILESYSTEM);
dirp->change_dir(base);
dirp->list_dir_begin();
Error err = dirp->list_dir_begin();

if (!dirp) {
if (err != OK) {
return false;
}

Expand Down
4 changes: 2 additions & 2 deletions scene/gui/text_edit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,8 @@ 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe make it a fatal error rather than a warning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that by removing the explicit handling, the container implementation will crash (on purpose) on index-out-of-bounds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to crash here though, for debugging purposes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 here

This 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.

Copy link
Member

@akien-mga akien-mga Aug 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

static Map<int, ColorRegionInfo> cri;
ERR_FAIL_INDEX_V(p_line, text.size(), cri);

if (text[p_line].width_cache == -1) {
_update_line_cache(p_line);
Expand Down
2 changes: 1 addition & 1 deletion scene/gui/tree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2868,8 +2868,8 @@ TreeItem *Tree::create_item(TreeItem *p_parent) {

TreeItem *ti = memnew(TreeItem(this));

ti->cells.resize(columns.size());
ERR_FAIL_COND_V(!ti, NULL);
ti->cells.resize(columns.size());

if (p_parent) {

Expand Down
9 changes: 3 additions & 6 deletions servers/physics/broad_phase_basic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,14 @@
#include "broad_phase_basic.h"
#include "list.h"
#include "print_string.h"
BroadPhaseSW::ID BroadPhaseBasic::create(CollisionObjectSW *p_object_, int p_subindex) {
BroadPhaseSW::ID BroadPhaseBasic::create(CollisionObjectSW *p_object, int p_subindex) {

if (p_object_ == NULL) {

ERR_FAIL_COND_V(p_object_ == NULL, 0);
}
ERR_FAIL_COND_V(p_object == NULL, NULL);

current++;

Element e;
e.owner = p_object_;
e.owner = p_object;
e._static = false;
e.subindex = p_subindex;

Expand Down
14 changes: 8 additions & 6 deletions servers/visual/visual_server_canvas.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1099,13 +1099,15 @@ void VisualServerCanvas::canvas_light_occluder_set_polygon(RID p_occluder, RID p

if (occluder->polygon.is_valid()) {
LightOccluderPolygon *occluder_poly = canvas_light_occluder_polygon_owner.get(p_polygon);
if (!occluder_poly)
if (!occluder_poly) {
occluder->polygon = RID();
ERR_FAIL_COND(!occluder_poly);
occluder_poly->owners.insert(occluder);
occluder->polygon_buffer = occluder_poly->occluder;
occluder->aabb_cache = occluder_poly->aabb;
occluder->cull_cache = occluder_poly->cull_mode;
ERR_FAIL_COND(!occluder_poly);
} else {
occluder_poly->owners.insert(occluder);
occluder->polygon_buffer = occluder_poly->occluder;
occluder->aabb_cache = occluder_poly->aabb;
occluder->cull_cache = occluder_poly->cull_mode;
}
}
}
void VisualServerCanvas::canvas_light_occluder_set_transform(RID p_occluder, const Transform2D &p_xform) {
Expand Down
7 changes: 3 additions & 4 deletions servers/visual/visual_server_scene.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1482,11 +1482,10 @@ void VisualServerScene::_render_scene(const Transform p_cam_transform, const Cam

if (light && p_shadow_atlas.is_valid() && VSG::storage->light_has_shadow(E->get()->base)) {
lights_with_shadow[directional_shadow_count++] = E->get();
}

//add to list

directional_light_ptr[directional_light_count++] = light->instance;
//add to list
directional_light_ptr[directional_light_count++] = light->instance;
}
}

VSG::scene_render->set_directional_shadow_count(directional_shadow_count);
Expand Down