Fix some overflows and unitialized variables #33583

qarmin · 2019-11-12T22:09:08Z

Fixes also warnings showed by undefined behavior sanitizer like division by zero, unitialized variables and loss value conversions

scene/gui/range.cpp:188:10: runtime error: division by zero
editor/script_editor_debugger.cpp:1933:9: runtime error: load of value 3200171710, which is not a valid value for type 'ScriptEditorDebugger::CameraOverride'
scene/resources/texture.cpp:2588:15: runtime error: load of value 100, which is not a valid value for type 'CameraServer::FeedImage'
editor/editor_sectioned_inspector.cpp:311:7: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'ObjectID' (aka 'unsigned long') changed the value to 18446744073709551615 (64-bit, unsigned)
core/ustring.cpp:1419:17: runtime error: implicit conversion from type 'char' of value -60 (8-bit, signed) to type 'uint8_t' (aka 'unsigned char') changed the value to 196 (8-bit, unsigned)
scene/2d/tile_map.cpp:1236:15: runtime error: implicit conversion from type 'uint16_t' (aka 'unsigned short') of value 65129 (16-bit, unsigned) to type 'int16_t' (aka 'short') changed the value to -407 (16-bit, signed)
core/variant.cpp:2690:11: runtime error: implicit conversion from type 'int64_t' (aka 'long') of value -1 (64-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned)
scene/resources/audio_stream_sample.cpp:98:15: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'uint32_t' (aka 'unsigned int')
scene/resources/audio_stream_sample.cpp:475:20: runtime error: division by zero
core/math/camera_matrix.cpp:187:24: runtime error: division by zero
core/math/camera_matrix.cpp:188:24: runtime error: division by zero
core/math/camera_matrix.cpp:190:32: runtime error: division by zero
core/math/camera_matrix.cpp:191:32: runtime error: division by zero
core/math/camera_matrix.cpp:192:31: runtime error: division by zero
core/math/camera_matrix.cpp:193:33: runtime error: division by zero

and this leak

Direct leak of 104 byte(s) in 1 object(s) allocated from:
    #0 0x7f7f9be78ae8 in malloc (/lib/x86_64-linux-gnu/libasan.so.5+0x10dae8)
    #1 0x9c332bd in Memory::alloc_static(unsigned long, bool) core/os/memory.cpp:85
    #2 0x9c331bc in operator new(unsigned long, char const*) core/os/memory.cpp:42
    #3 0x9eb5781 in PackedSourcePCK::get_file(String const&, PackedData::PackedFile*) core/io/file_access_pack.cpp:220
    #4 0x9b64ea9 in PackedData::try_open_path(String const&) core/io/file_access_pack.h:187
    #5 0x9b5768d in FileAccess::open(String const&, int, Error*) core/os/file_access.cpp:100
    #6 0x9764db3 in ProjectSettings::_load_settings_binary(String const&) core/project_settings.cpp:493
    #7 0x97691c1 in ProjectSettings::_load_settings_text_or_binary(String const&, String const&) core/project_settings.cpp:606
    #8 0x9760b60 in ProjectSettings::_setup(String const&, String const&, bool) core/project_settings.cpp:392
    #9 0x9764335 in ProjectSettings::setup(String const&, String const&, bool) core/project_settings.cpp:467
    #10 0x1098c89 in Main::setup(char const*, int, char**, bool) main/main.cpp:825
    #11 0xf3e2ee in main platform/x11/godot_x11.cpp:49
    #12 0x7f7f9a9541e2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x271e2)

core/image.cpp

core/io/packet_peer.cpp

akien-mga · 2019-11-14T09:08:13Z

core/project_settings.cpp

@@ -525,6 +525,8 @@ Error ProjectSettings::_load_settings_binary(const String &p_path) {
 		set(key, value);
 	}

+	f->close();


Do we really need to close before memdelete? Because if so there are tons of locations where this is not done currently and we just memdelete(f) (including in this very method).

core/variant.cpp

akien-mga · 2019-11-14T09:28:25Z

drivers/png/image_loader_png.cpp

@@ -64,7 +65,7 @@ Ref<Image> ImageLoaderPNG::load_mem_png(const uint8_t *p_png, int p_size) {
 	Ref<Image> img;
 	img.instance();

-	Error err = PNGDriverCommon::png_to_image(p_png, p_size, img);
+	Error err = PNGDriverCommon::png_to_image(p_png, p_size == -1 ? (sizeof(size_t) == 4 ? INT32_MAX : INT64_MAX) : p_size, img);


Again, overflowing the integer size is NOT a feature we use (at least not in such obvious case), we shouldn't make code that does it explicitly to silence a warning pointing us to an actual bug.

p_size should be unsigned, and lossless_unpack_png should be fixed so that it can't pass a negative size (which should already be the case BTW since it errors if len < 4 and it passes len - 4, so I'm not sure what we're fixing here).

Edit: load_mem_png is passed around as _png_mem_loader_func, it must be one of those downstream users that pass invalid sizes and they should all be reviewed and fixed.

Just do something like:

ERR_FAIL_COND_V_MSG(p_size < 0, Ref<Image>, "Cannot convert PNG to Image resource with a negative size.");

I can't put that line there, because Godot crash a little after start in OS_X11::set_icon function

Default value of -1 is set to p_len and is used in a lot of functions using Image with non default constructor

This needs a dedicated bug report, but this manual overflow is definitely not OK.
Image(const uint8_t *p_mem_png_jpg, int p_len = -1); is bogus and should be removed, or changed so that a default length of -1 means that the size is inferred from the byte array. Currently that's not done, so I don't even understand how make_icon works at all.

editor/editor_sectioned_inspector.cpp

scene/gui/range.cpp

drivers/gles2/rasterizer_canvas_gles2.cpp

editor/editor_sectioned_inspector.cpp

scene/gui/range.cpp

core/variant.cpp

aaronfranke · 2019-11-18T21:34:23Z

core/typedefs.h

@@ -174,6 +174,9 @@ inline void __swap_tmpl(T &x, T &y) {

 static _FORCE_INLINE_ unsigned int next_power_of_2(unsigned int x) {

+	if (x == 0)
+		return 0;


Braces will be enforced in the near future, see #33027

Yeah but this will be done by a script. No need to refactor existing code to match it yet (it's not bad to do it either, but it's not necessary).

aaronfranke · 2019-11-18T21:34:51Z

scene/animation/tween.cpp

@@ -63,6 +63,8 @@ void Tween::_add_pending_command(StringName p_key, const Variant &p_arg1, const
 		count = 2;
 	else if (p_arg1.get_type() != Variant::NIL)
 		count = 1;
+	else
+		count = 0;


Same as above.

core/math/camera_matrix.cpp

akien-mga · 2019-11-19T07:06:17Z

drivers/gles3/rasterizer_canvas_gles3.cpp

@@ -820,7 +820,7 @@ void RasterizerCanvasGLES3::_canvas_item_render_commands(Item *p_item, Item *cur

 				RasterizerStorageGLES3::Texture *texture = _bind_canvas_texture(mesh->texture, mesh->normal_map);

-				if (texture) {
+				if (texture && texture->width != 0 && texture->height != 0) {


There are lots of other occurrences of the same code in this file which also need to be handled.

I don't know if this would negatively impact performance, maybe we'd better ensure that textures can't have null widths/heights? CC @clayjohn

This one is the last change I'm not 100% sure about, so I'd suggest to move it to a separate PR to be discussed further. Then this PR can be merged.

akien-mga · 2019-11-20T20:31:19Z

Thanks!

qarmin requested a review from reduz as a code owner November 12, 2019 22:09

Calinou added bug topic:core labels Nov 12, 2019

akien-mga added this to the 3.2 milestone Nov 12, 2019

qarmin force-pushed the fix_overflows_unitialized branch 4 times, most recently from 14c27bb to 82a2ae8 Compare November 13, 2019 16:48