From 046f47bcc57a9003f926a1919fa530af980b3982 Mon Sep 17 00:00:00 2001 From: Clifford Yapp <238416+starseeker@users.noreply.github.com> Date: Wed, 8 Dec 2021 17:41:54 -0500 Subject: [PATCH] Make the fb names static, to match dm names Oof. Writing down the details on this one, as I doubt I'll remember... The start was turning on both the AddressSanitizer and Qt. When doing so, most programs suddenly began reporting memory leaks. However, the report was rather cryptic, being four entries similar to this one: ==1262202==ERROR: LeakSanitizer: detected memory leaks Direct leak of 21 byte(s) in 1 object(s) allocated from: #0 0x498087 in posix_memalign (build/src/libdm/tests/dm_test+0x498087) #1 0x7f4e90e53675 in alloc brlcad/src/libbu/malloc.c:129:10 #2 0x7f4e90e53400 in bu_malloc brlcad/src/libbu/malloc.c:167:12 #3 0x7f4e90ee1c02 in bu_strdupm brlcad/src/libbu/str.c:165:17 #4 0x7f4e87dcd3e9 () #5 0x7f4e87dcdea5 () #6 0x7f4e9650eb89 (/lib64/ld-linux-x86-64.so.2+0x11b89) I wasn't immediately sure if the unknown module error was related to the plugin loading, and spent a lot of time trying to find a string memory issue in dm_init.cpp. After that proved fruitless, other than to confirm that the error disappeared if i removed the bu_dlclose-ing of the handles saved at initialization, I began to look for ways to decode the "unknown module" entries. That led to the following issue: https://github.com/google/sanitizers/issues/89 which describes the problem ASan has with dynamic libraries. Comments indicated that it would see real issues, but won't report which dynamic library they come from (and there are no plans to fix this anytime soon... grr...). In fairness, valgrind can see the error too but also has the same reporting problem; it appears to be ubiquitous. Fortunately, we have an alternative due to the way our plugin system and test apps work - we can simply add and remove .so files to the directory and see how the error reporting changes to zero in on which file(s) are triggering the problem. Doing so quickly made apparent what should have been obvious in retrospect - two of the errors each were coming from the Qt and swrast plugins, which had been off in earlier testing. Since there was no backtrace beyond the bu_strdupm call itself, and there were two errors per file, the suspect was the bu_strdup calls initializing the "char *" names for the fb structures. The C files use static strings (which is why the non-Qt plugins didn't show the issue), but C++ doesn't tolerate the type mismatch. The original hack workaround was just to bu_strdup and create a (char *) string, but as the leak detectors correctly note this also means there's no way to clean up the allocated memory. As far as I can tell there is no reason for these strings to be editable (char *) strings - the dm container's equivalents are static. This commit removes any logic assuming if_name is dynamic, and also removes the bu_strdup hack from Qt and swrast. --- include/dm.h | 3 +-- src/libdm/dm_plugins.cpp | 10 ---------- src/libdm/fb_generic.c | 11 +---------- src/libdm/include/calltable.h | 4 ++-- src/libdm/qtgl/fb-qtgl.cpp | 4 ++-- src/libdm/swrast/fb-swrast.cpp | 4 ++-- 6 files changed, 8 insertions(+), 28 deletions(-) diff --git a/include/dm.h b/include/dm.h index fdfeb9c2d59..d1c415f75c6 100644 --- a/include/dm.h +++ b/include/dm.h @@ -372,7 +372,7 @@ DM_EXPORT struct fb *fb_get(); DM_EXPORT struct fb *fb_raw(const char *type); DM_EXPORT void fb_put(struct fb *ifp); DM_EXPORT struct dm *fb_get_dm(struct fb *ifp); -DM_EXPORT extern char *fb_gettype(struct fb *ifp); +DM_EXPORT extern const char *fb_gettype(struct fb *ifp); DM_EXPORT extern void fb_set_standalone(struct fb *ifp, int val); DM_EXPORT extern int fb_get_standalone(struct fb *ifp); DM_EXPORT extern int fb_get_max_width(struct fb *ifp); @@ -431,7 +431,6 @@ DM_EXPORT extern icv_image_t *fb_write_icv(struct fb *ifp, int scr_xoff, int scr DM_EXPORT extern int fb_read_png(struct fb *ifp, FILE *fp_in, int file_xoff, int file_yoff, int scr_xoff, int scr_yoff, int clear, int zoom, int inverse, int one_line_only, int multiple_lines, int verbose, int header_only, double def_screen_gamma, struct bu_vls *result); DM_EXPORT extern int fb_set_interface(struct fb *ifp, const char *interface_type); -DM_EXPORT extern void fb_set_name(struct fb *ifp, const char *name); DM_EXPORT extern const char *fb_get_name(const struct fb *ifp); DM_EXPORT extern void fb_set_magic(struct fb *ifp, uint32_t magic); DM_EXPORT extern long fb_get_pagebuffer_pixel_size(struct fb *ifp); diff --git a/src/libdm/dm_plugins.cpp b/src/libdm/dm_plugins.cpp index 517809a1fac..ad06a28ff50 100644 --- a/src/libdm/dm_plugins.cpp +++ b/src/libdm/dm_plugins.cpp @@ -465,22 +465,12 @@ fb_open(const char *file, int width, int height) } found_interface: - /* Copy over the name it was opened by. */ - ifp->i->if_name = (char*)malloc((unsigned) strlen(file) + 1); - if (!ifp->i->if_name) { - Malloc_Bomb(strlen(file) + 1); - free((void *) ifp); - return FB_NULL; - } - bu_strlcpy(ifp->i->if_name, file, strlen(file)+1); - /* Mark OK by filling in magic number */ ifp->i->if_magic = FB_MAGIC; i = (*ifp->i->if_open)(ifp, file, width, height); if (i != 0) { ifp->i->if_magic = 0; /* sanity */ - free((void *) ifp->i->if_name); free((void *) ifp); if (i < 0) diff --git a/src/libdm/fb_generic.c b/src/libdm/fb_generic.c index 3da497e2d88..4b422f1ff3b 100644 --- a/src/libdm/fb_generic.c +++ b/src/libdm/fb_generic.c @@ -59,7 +59,6 @@ struct fb *fb_get() struct fb *new_fb = FB_NULL; BU_GET(new_fb, struct fb); BU_GET(new_fb->i, struct fb_impl); - new_fb->i->if_name = NULL; return new_fb; } @@ -133,13 +132,6 @@ fb_configure_window(struct fb *ifp, int width, int height) return ifp->i->if_configure_window(ifp, width, height); } -void fb_set_name(struct fb *ifp, const char *name) -{ - if (!ifp) return; - ifp->i->if_name = (char *)bu_malloc((unsigned)strlen(name)+1, "if_name"); - bu_strlcpy(ifp->i->if_name, name, strlen(name)+1); -} - const char *fb_get_name(const struct fb *ifp) { if (!ifp) return NULL; @@ -194,7 +186,7 @@ struct dm *fb_get_dm(struct fb *ifp) return ifp->i->dmp; } -char *fb_gettype(struct fb *ifp) +const char *fb_gettype(struct fb *ifp) { return ifp->i->if_type; } @@ -345,7 +337,6 @@ fb_close(struct fb *ifp) } if (ifp->i->if_pbase != PIXEL_NULL) free((void *) ifp->i->if_pbase); - free((void *) ifp->i->if_name); free((void *) ifp->i); free((void *) ifp); return 0; diff --git a/src/libdm/include/calltable.h b/src/libdm/include/calltable.h index 0efa5614651..9dd46621787 100644 --- a/src/libdm/include/calltable.h +++ b/src/libdm/include/calltable.h @@ -188,11 +188,11 @@ struct fb_impl { int (*if_flush)(struct fb *ifp); /**< @brief flush output */ int (*if_free)(struct fb *ifp); /**< @brief free resources */ int (*if_help)(struct fb *ifp); /**< @brief print useful info */ - char *if_type; /**< @brief what "open" calls it */ + const char *if_type; /**< @brief what "open" calls it */ int if_max_width; /**< @brief max device width */ int if_max_height; /**< @brief max device height */ /* Dynamic information: per device INSTANCE. */ - char *if_name; /**< @brief what the user called it */ + const char *if_name; /**< @brief what the user called it */ int if_width; /**< @brief current values */ int if_height; int if_selfd; /**< @brief select(fd) for input events if >= 0 */ diff --git a/src/libdm/qtgl/fb-qtgl.cpp b/src/libdm/qtgl/fb-qtgl.cpp index ff2a0df8ec3..6d26ff7676b 100644 --- a/src/libdm/qtgl/fb-qtgl.cpp +++ b/src/libdm/qtgl/fb-qtgl.cpp @@ -983,10 +983,10 @@ struct fb_impl qtgl_interface_impl = qtgl_flush, /* flush output */ qtgl_free, /* free resources */ qtgl_help, /* help message */ - bu_strdup("Qt OpenGL"), /* device description */ + "Qt OpenGL", /* device description */ FB_XMAXSCREEN, /* max width */ FB_YMAXSCREEN, /* max height */ - bu_strdup("/dev/qtgl"), /* short device name */ + "/dev/qtgl", /* short device name */ 512, /* default/current width */ 512, /* default/current height */ -1, /* select file desc */ diff --git a/src/libdm/swrast/fb-swrast.cpp b/src/libdm/swrast/fb-swrast.cpp index 96b4ffd544b..30b8b618a06 100644 --- a/src/libdm/swrast/fb-swrast.cpp +++ b/src/libdm/swrast/fb-swrast.cpp @@ -965,10 +965,10 @@ struct fb_impl swrast_interface_impl = swrast_flush, /* flush output */ swrast_free, /* free resources */ swrast_help, /* help message */ - bu_strdup("OSMesa swrast OpenGL"), /* device description */ + "OSMesa swrast OpenGL", /* device description */ FB_XMAXSCREEN, /* max width */ FB_YMAXSCREEN, /* max height */ - bu_strdup("/dev/swrast"), /* short device name */ + "/dev/swrast", /* short device name */ 512, /* default/current width */ 512, /* default/current height */ -1, /* select file desc */