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

Jump to bottom

Error handling #13

Merged
merged 6 commits into from
Dec 14, 2019
Merged

Error handling #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
97bb961
fix possible memory leak (issue #8) and improve error handling
osch Dec 11, 2019
3a8cced
fix typo in testcase
osch Dec 11, 2019
acbf9fc
throw lua error on malloc failures instead calling assert
osch Dec 11, 2019
adf832f
improved error message
osch Dec 11, 2019
271ab5e
refactored function "from_lua_rectangle2" to "from_lua_rectangle_list…
osch Dec 14, 2019
362f4d4
check for rectangle integer coordinates under Lua5.3
osch Dec 14, 2019
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
8 changes: 6 additions & 2 deletions obj_context.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,9 @@ cr_get_dash (lua_State *L) {
cnt = cairo_get_dash_count(*obj);
if (cnt > 0) {
dashes = malloc(sizeof(double) * cnt);
assert(dashes);
if (!dashes) {
return luaL_error(L, "out of memory");
}
}

cairo_get_dash(*obj, dashes, &offset);
Expand Down Expand Up @@ -642,7 +644,9 @@ cr_set_dash (lua_State *L) {
num_dashes = lua_objlen(L, 2);
if (num_dashes > 0) {
dashes = malloc(sizeof(double) * num_dashes);
assert(dashes);
if (!dashes) {
return luaL_error(L, "out of memory");
}
dashtotal = 0;

for (i = 0; i < num_dashes; ++i) {
Expand Down
4 changes: 3 additions & 1 deletion obj_font_face.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,9 @@ user_font_face_create (lua_State *L) {

lua_createtable(L, 4, 0);
info = malloc(sizeof(UserFontInfo));
assert(info);
if (!info) {
return luaL_error(L, "out of memory");
}
info->L = L;
info->ref = LUA_NOREF;

Expand Down
19 changes: 8 additions & 11 deletions obj_region.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,23 +39,20 @@ region_create_rectangle (lua_State *L) {

static int
region_create_rectangles (lua_State *L) {
size_t max_entries = lua_objlen (L, 1);
cairo_rectangle_int_t *rects = calloc(max_entries, sizeof(*rects));
size_t i = 0;
luaL_checktype(L, 1, LUA_TTABLE);
lua_settop(L, 1); /* discard other args */

size_t max_entries = lua_objlen (L, 1);
cairo_rectangle_int_t *rects = lua_newuserdata(L, max_entries * sizeof(cairo_rectangle_int_t));
size_t i;

/* Now iterate over the table */
lua_pushnil(L);
while (lua_next(L, 1) != 0) {
if (i >= max_entries)
return luaL_error(L, "Cairo.region_create_rectangles(): Internal error, table larger than table?!");
from_lua_rectangle (L, &rects[i], 3);
i++;
lua_pop(L, 1);
for (i = 0; i < max_entries; ++i) {
from_lua_rectangle_list_element(L, &rects[i], 1, i+1);
}

cairo_region_t **reg = create_region_userdata(L);
*reg = cairo_region_create_rectangles(rects, i);
free(rects);
return 1;
}

Expand Down
10 changes: 7 additions & 3 deletions obj_surface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,9 @@ image_surface_create_from_data (lua_State *L) {

surface = create_surface_userdata(L);
surface->image_buffer = malloc(data_len);
assert(surface->image_buffer);
if (!surface->image_buffer) {
return luaL_error(L, "out of memory");
}
memcpy(surface->image_buffer, data, data_len);
surface->surface = cairo_image_surface_create_for_data(
surface->image_buffer, fmt, width, height, stride);
Expand Down Expand Up @@ -474,7 +476,9 @@ surface_get_gdk_pixbuf (lua_State *L) {
strideo = ((3 * width) + 7) & ~7; /* align to 8 bytes */
buffer_len = strideo * height;
buffer = malloc(buffer_len);
assert(buffer);
if (!buffer) {
return luaL_error(L, "out of memory");
}

rowpi = (const char *) cairo_image_surface_get_data(*surface);
rowpo = buffer;
Expand Down Expand Up @@ -719,7 +723,7 @@ set_mime_data(lua_State *L)
mime_data = luaL_checklstring(L, 3, &data_length);
mime_priv = malloc(data_length);
if (!mime_priv) {
return push_cairo_status(L, CAIRO_STATUS_NO_MEMORY);
return luaL_error(L, "out of memory");
}
memcpy(mime_priv, mime_data, data_length);
}
Expand Down
46 changes: 41 additions & 5 deletions oocairo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ static int luaL_typerror (lua_State *L, int narg, const char *tname) {
#define lua_rawlen(a, b) do_not_use_lua_rawlen_use_lua_objlen_instead;
#endif

#if LUA_VERSION_NUM >= 503
#define oocairo_lua_isinteger(L, idx) lua_isinteger(L, idx)
#else
#define oocairo_lua_isinteger(L, idx) lua_isnumber(L, idx)
#endif

static const int ENDIANNESS_TEST_VAL = 1;
#define IS_BIG_ENDIAN (!(*(const char *) &ENDIANNESS_TEST_VAL))

Expand Down Expand Up @@ -363,12 +369,37 @@ from_lua_rectangle (lua_State *L, cairo_rectangle_int_t *rect, int pos) {
#define DO(t) \
lua_pushstring(L, #t); \
lua_gettable(L, pos); \
rect->t = luaL_checkinteger(L, -1); \
if (!oocairo_lua_isinteger(L, -1)) { \
luaL_argerror(L, pos, "invalid rectangle: coordinates must be integer values"); \
} \
rect->t = lua_tointeger(L, -1); \
Copy link
Member

Choose a reason for hiding this comment

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

Hm... With Lua 5.3, luaL_checkinteger does more than lua_isnumber, because 5.5 is a number, but not an integer. But lua_isinteger only exists in Lua 5.3 and not in older Lua versions.

How about something like this:

#if LUA_VERSION_NUM >= 503
#define oocairo_lua_isinteger(L, idx) lua_isinteger(L, idx)
#else
#define oocairo_lua_isinteger(L, idx) lua_isnumber(L, idx)
#endif

One could also check if the number is an integer in older Lua versions instead of silently truncating, but I am not sure if that is worth it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Lua 5.1 documentation for luaL_checkinteger differs from the Lua 5.3 documentation:

Lua 5.1

luaL_checkinteger
Checks whether the function argument narg is a number and returns this number cast to a lua_Integer.

Lua 5.3

luaL_checkinteger
Checks whether the function argument arg is an integer (or can be converted to an integer) and returns this integer cast to a lua_Integer.

So if the documentation is true there was already a silent truncation of numbers to integers before my changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I incorporated oocairo_lua_isinteger as you suggested, see my latest commit.

lua_pop(L, 1)
DO(x);
DO(y);
DO(width);
DO(height);
#undef DO
}

static void
from_lua_rectangle_list_element (lua_State *L, cairo_rectangle_int_t *rect, int list_pos, int element_index) {
lua_rawgeti(L, list_pos, element_index); /* push rectangle from list */
if (lua_type(L, -1) != LUA_TTABLE) {
luaL_argerror(L, list_pos, "list contains invalid rectangle");
}
#define DO(t) \
lua_pushstring(L, #t); \
lua_gettable(L, -2); \
if (!oocairo_lua_isinteger(L, -1)) { \
luaL_argerror(L, list_pos, "list contains invalid rectangle: coordinates must be integer values"); \
} \
rect->t = lua_tointeger(L, -1); \
lua_pop(L, 1)
DO(x);
DO(y);
DO(width);
DO(height);
lua_pop(L, 1); /* pop rectangle */
#undef DO
}
#endif
Expand Down Expand Up @@ -494,7 +525,9 @@ from_lua_glyph_array (lua_State *L, cairo_glyph_t **glyphs, int *num_glyphs,
return;
}
*glyphs = GLYPHS_ALLOC(*num_glyphs);
assert(*glyphs);
if (!*glyphs) {
return luaL_error(L, "out of memory");
}

for (i = 0; i < *num_glyphs; ++i) {
lua_rawgeti(L, pos, i + 1);
Expand Down Expand Up @@ -578,7 +611,9 @@ from_lua_clusters_table (lua_State *L, cairo_text_cluster_t **clusters,
return;
}
*clusters = cairo_text_cluster_allocate(*num);
assert(*clusters);
if (!*clusters) {
return luaL_error(L, "out of memory");
}

for (i = 0; i < *num; ++i) {
lua_rawgeti(L, pos, i + 1);
Expand Down Expand Up @@ -778,8 +813,9 @@ free_surface_userdata (SurfaceUserdata *ud) {
static char *
my_strdup (const char *s) {
char *copy = malloc(strlen(s) + 1);
assert(copy);
strcpy(copy, s);
if (copy) {
strcpy(copy, s);
}
Copy link
Member

Choose a reason for hiding this comment

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

Memo to myself: Get rid of my_strdup() and just use strdup() instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let us keep it as it is, because strdup is only C2x or Posix standard conform. I just experienced in another project very confusing problems using strdup (or _strdup?) under windows. Such a small function is not worth the hassle.

return copy;
}

Expand Down
54 changes: 54 additions & 0 deletions test/region.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ if Cairo.check_version(1, 10, 0) then
assert_equal(reg:contains_rectangle(rect), "overlap-in")
rect.width = 8
assert_equal(reg:contains_rectangle(rect), "overlap-part")
local ok, errmsg = pcall(function()
reg:contains_rectangle({ x = 0, y = 0 })
end)
assert_false(ok)
assert_match("bad argument %#1 to %'contains_rectangle%' %(invalid rectangle%: coordinates must be integer values%)", errmsg)
end

function module.test_copy ()
Expand Down Expand Up @@ -136,6 +141,55 @@ if Cairo.check_version(1, 10, 0) then
assert_equal(rect[2].y, 10)
assert_equal(rect[2].width, 1)
assert_equal(rect[2].height, 1)

local reg = Cairo.region_create_rectangles(rects, "unused")
local rect = reg:get_rectangles()
assert_equal(#rect, 2)
assert_equal(rect[1].x, 0)
assert_equal(rect[1].y, 0)
assert_equal(rect[1].width, 1)
assert_equal(rect[1].height, 1)
assert_equal(rect[2].x, 10)
assert_equal(rect[2].y, 10)
assert_equal(rect[2].width, 1)
assert_equal(rect[2].height, 1)

local rects = { rect1, rect2, unused = true }
local reg = Cairo.region_create_rectangles(rects)
local rect = reg:get_rectangles()
assert_equal(#rect, 2)
assert_equal(rect[1].x, 0)
assert_equal(rect[1].y, 0)
assert_equal(rect[1].width, 1)
assert_equal(rect[1].height, 1)
assert_equal(rect[2].x, 10)
assert_equal(rect[2].y, 10)
assert_equal(rect[2].width, 1)
assert_equal(rect[2].height, 1)

local rect3 = { x = 0, y = 0 }
local rects = { rect1, rect2, rect3 }
local ok, errmsg = pcall(function()
Cairo.region_create_rectangles(rects)
end)
assert_false(ok)
assert_match("bad argument %#1 to %'region_create_rectangles%' %(list contains invalid rectangle%: coordinates must be integer values%)", errmsg)

local rect3 = { x = 0, y = 0, width = 10, height = {} }
local rects = { rect1, rect2, rect3 }
local ok, errmsg = pcall(function()
Cairo.region_create_rectangles(rects)
end)
assert_false(ok)
assert_match("bad argument %#1 to %'region_create_rectangles%' %(list contains invalid rectangle%: coordinates must be integer values%)", errmsg)

local rects = { rect1, rect2, "" }
local ok, errmsg = pcall(function()
Cairo.region_create_rectangles(rects)
end)
assert_false(ok)
assert_match("bad argument %#1 to %'region_create_rectangles%' %(list contains invalid rectangle%)", errmsg)

end
end

Expand Down
4 changes: 3 additions & 1 deletion test/surface.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,9 @@ if Cairo.check_version(1, 10, 0) then
test("test/data", "test data")

local surface = Cairo.image_surface_create("rgb24", 0, 0)
assert_error(function () surface_set_mime_data(nil, "foo") end)
local ok, errmsg = pcall(function () surface:set_mime_data(nil, "foo") end)
assert_false(ok)
assert_match("bad argument %#1 to %'set_mime_data%' %(string expected%, got nil%)", errmsg)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a typo? The commit message confuses me.

Ah, surface_set_mime_data does not exist and thus this errored for the wrong reason. That could be made clearer in the commit message, I think...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How could this be made clearer? The commit message explains what the commit is for. One could double the code diff in the commit message, for example "fixed typo in testcase: call surface:set_mime_data instead of undefined surface_set_mime_data". Is this better? However there are onle three lines in the commit so it should not be too hard to understand what this commit is doing.

end
end

Expand Down