From aa0b2bb5fcb9bc7e52049f380172d6d3c2baf5ec Mon Sep 17 00:00:00 2001 From: Francesco Abbate Date: Fri, 21 May 2021 23:22:46 +0200 Subject: [PATCH] Fix crash problem with rencache and font access In some cases rencache was using a FontDesc pointer that was actually freed by Lua giving segfaults errors. In addition, some FontDesc object were not freed in some cases if the rencache_end_frame was not called when performing the "restart" command. The invalid access problem can happen because rencache keep some pointers to FontDesc object but these are Lua userdata that Lua can dispose of. This situation is prone to hard errors and we should avoid to keep pointers to objects managed by Lua. To this purpose we use luaL_ref/unref to bind the FontDesc into the Lua's registry while rencache need them. We still keeps pointer to FontDesc object but using luaL_ref we are assured they will not be disposed by Lua. Since we are using luaL_ref/unref to inform the GC about when the objects are in use we can now finalize the objects directly when Lua collects them. Previously the GC metamethods was issuing a FREE command to rencache and the font was actually freed only from the rencache_end_frame function. --- src/api/renderer.c | 6 ++-- src/api/renderer_font.c | 5 ++- src/main.c | 5 --- src/rencache.c | 72 +++++++++++++++++++++++++---------------- src/rencache.h | 9 +++--- 5 files changed, 55 insertions(+), 42 deletions(-) diff --git a/src/api/renderer.c b/src/api/renderer.c index c6883a95..8dc13ada 100644 --- a/src/api/renderer.c +++ b/src/api/renderer.c @@ -38,13 +38,13 @@ static int f_get_size(lua_State *L) { static int f_begin_frame(lua_State *L) { - rencache_begin_frame(); + rencache_begin_frame(L); return 0; } static int f_end_frame(lua_State *L) { - rencache_end_frame(); + rencache_end_frame(L); return 0; } @@ -90,7 +90,7 @@ static int draw_text_subpixel_impl(lua_State *L, bool draw_subpixel) { replace_color = (RenColor) {0}; } - x_subpixel = rencache_draw_text(font_desc, text, x_subpixel, y, color, draw_subpixel, rep_table, replace_color); + x_subpixel = rencache_draw_text(L, font_desc, 1, text, x_subpixel, y, color, draw_subpixel, rep_table, replace_color); lua_pushnumber(L, x_subpixel); return 1; } diff --git a/src/api/renderer_font.c b/src/api/renderer_font.c index 3d85597b..47ea97a3 100644 --- a/src/api/renderer_font.c +++ b/src/api/renderer_font.c @@ -1,3 +1,6 @@ +#include +#include + #include "api.h" #include "fontdesc.h" #include "renderer.h" @@ -62,7 +65,7 @@ static int f_set_tab_size(lua_State *L) { static int f_gc(lua_State *L) { FontDesc *self = luaL_checkudata(L, 1, API_TYPE_FONT); - rencache_free_font(self); + font_desc_free(self); return 0; } diff --git a/src/main.c b/src/main.c index ca332dd6..b0eb50d6 100644 --- a/src/main.c +++ b/src/main.c @@ -159,11 +159,6 @@ init_lua: enable_momentum_scroll(); #endif - /* We need to clear the rencache commands because we may restarting the application - and it could be non-empty. It is important to clear the command buffer because it - stores pointers to font_desc objects and these are Lua managed. */ - rencache_clear(); - const char *init_lite_code = \ "local core\n" "xpcall(function()\n" diff --git a/src/rencache.c b/src/rencache.c index 1f3e42c2..31165e90 100644 --- a/src/rencache.c +++ b/src/rencache.c @@ -1,6 +1,8 @@ #include #include #include + +#include #include "rencache.h" /* a cache over the software renderer -- all drawing operations are stored as @@ -14,7 +16,7 @@ #define COMMAND_BUF_SIZE (1024 * 512) #define COMMAND_BARE_SIZE offsetof(Command, text) -enum { FREE_FONT, SET_CLIP, DRAW_TEXT, DRAW_RECT, DRAW_TEXT_SUBPIXEL }; +enum { SET_CLIP, DRAW_TEXT, DRAW_RECT, DRAW_TEXT_SUBPIXEL }; typedef struct { int8_t type; @@ -30,6 +32,15 @@ typedef struct { char text[0]; } Command; +#define FONT_REFS_MAX 12 +struct FontRef { + FontDesc *font_desc; + int index; +}; +typedef struct FontRef FontRef; +FontRef font_refs[FONT_REFS_MAX]; +int font_refs_len = 0; + static unsigned cells_buf1[CELLS_X * CELLS_Y]; static unsigned cells_buf2[CELLS_X * CELLS_Y]; @@ -45,6 +56,33 @@ static bool show_debug; static inline int min(int a, int b) { return a < b ? a : b; } static inline int max(int a, int b) { return a > b ? a : b; } +static int font_refs_add(lua_State *L, FontDesc *font_desc, int index) { + for (int i = 0; i < font_refs_len; i++) { + if (font_refs[i].font_desc == font_desc) { + return font_refs[i].index; + } + } + + if (font_refs_len >= FONT_REFS_MAX) { + fprintf(stderr, "Warning: (" __FILE__ "): exhausted font reference buffer\n"); + return LUA_NOREF; + } + + lua_pushvalue(L, index); + int ref = luaL_ref(L, LUA_REGISTRYINDEX); + + font_refs[font_refs_len++] = (FontRef) { font_desc, ref }; + return ref; +} + + +static void font_refs_clear(lua_State *L) { + for (int i = 0; i < font_refs_len; i++) { + luaL_unref(L, LUA_REGISTRYINDEX, font_refs[i].index); + } + font_refs_len = 0; +} + /* 32bit fnv-1a hash */ #define HASH_INITIAL 2166136261 @@ -115,12 +153,6 @@ void rencache_show_debug(bool enable) { } -void rencache_free_font(FontDesc *font_desc) { - Command *cmd = push_command(FREE_FONT, COMMAND_BARE_SIZE); - if (cmd) { cmd->font_desc = font_desc; } -} - - void rencache_set_clip_rect(RenRect rect) { Command *cmd = push_command(SET_CLIP, COMMAND_BARE_SIZE); if (cmd) { cmd->rect = intersect_rects(rect, screen_rect); } @@ -136,7 +168,7 @@ void rencache_draw_rect(RenRect rect, RenColor color) { } } -int rencache_draw_text(FontDesc *font_desc, +int rencache_draw_text(lua_State *L, FontDesc *font_desc, int font_index, const char *text, int x, int y, RenColor color, bool draw_subpixel, CPReplaceTable *replacements, RenColor replace_color) { @@ -148,7 +180,7 @@ int rencache_draw_text(FontDesc *font_desc, rect.width = ren_font_subpixel_round(w_subpixel, subpixel_scale, 0); rect.height = ren_get_font_height(font_desc); - if (rects_overlap(screen_rect, rect)) { + if (rects_overlap(screen_rect, rect) && font_refs_add(L, font_desc, font_index) >= 0) { int sz = strlen(text) + 1; Command *cmd = push_command(draw_subpixel ? DRAW_TEXT_SUBPIXEL : DRAW_TEXT, COMMAND_BARE_SIZE + sz); if (cmd) { @@ -173,7 +205,7 @@ void rencache_invalidate(void) { } -void rencache_begin_frame(void) { +void rencache_begin_frame(lua_State *L) { /* reset all cells if the screen width/height has changed */ int w, h; ren_get_size(&w, &h); @@ -182,6 +214,7 @@ void rencache_begin_frame(void) { screen_rect.height = h; rencache_invalidate(); } + font_refs_clear(L); } @@ -214,7 +247,7 @@ static void push_rect(RenRect r, int *count) { } -void rencache_end_frame(void) { +void rencache_end_frame(lua_State *L) { /* update cells from commands */ Command *cmd = NULL; RenRect cr = screen_rect; @@ -253,7 +286,6 @@ void rencache_end_frame(void) { } /* redraw updated regions */ - bool has_free_commands = false; for (int i = 0; i < rect_count; i++) { /* draw */ RenRect r = rect_buf[i]; @@ -262,9 +294,6 @@ void rencache_end_frame(void) { cmd = NULL; while (next_command(&cmd)) { switch (cmd->type) { - case FREE_FONT: - has_free_commands = true; - break; case SET_CLIP: ren_set_clip_rect(intersect_rects(cmd->rect, r)); break; @@ -296,16 +325,6 @@ void rencache_end_frame(void) { ren_update_rects(rect_buf, rect_count); } - /* free fonts */ - if (has_free_commands) { - cmd = NULL; - while (next_command(&cmd)) { - if (cmd->type == FREE_FONT) { - font_desc_free(cmd->font_desc); - } - } - } - /* swap cell buffer and reset */ unsigned *tmp = cells; cells = cells_prev; @@ -313,6 +332,3 @@ void rencache_end_frame(void) { command_buf_idx = 0; } -void rencache_clear() { - command_buf_idx = 0; -} diff --git a/src/rencache.h b/src/rencache.h index 987b6855..1d0f45a6 100644 --- a/src/rencache.h +++ b/src/rencache.h @@ -2,17 +2,16 @@ #define RENCACHE_H #include +#include #include "renderer.h" void rencache_show_debug(bool enable); -void rencache_free_font(FontDesc *font_desc); void rencache_set_clip_rect(RenRect rect); void rencache_draw_rect(RenRect rect, RenColor color); -int rencache_draw_text(FontDesc *font_desc, const char *text, int x, int y, RenColor color, +int rencache_draw_text(lua_State *L, FontDesc *font_desc, int font_index, const char *text, int x, int y, RenColor color, bool draw_subpixel, CPReplaceTable *replacements, RenColor replace_color); void rencache_invalidate(void); -void rencache_begin_frame(void); -void rencache_end_frame(void); -void rencache_clear(); +void rencache_begin_frame(lua_State *L); +void rencache_end_frame(lua_State *L); #endif