Skip to content

Commit

Permalink
Make the fb names static, to match dm names
Browse files Browse the repository at this point in the history
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  (<unknown module>)
    #5 0x7f4e87dcdea5  (<unknown module>)
    #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:
google/sanitizers#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.
  • Loading branch information
starseeker committed Dec 8, 2021
1 parent de30936 commit 046f47b
Show file tree
Hide file tree
Showing 6 changed files with 8 additions and 28 deletions.
3 changes: 1 addition & 2 deletions include/dm.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
10 changes: 0 additions & 10 deletions src/libdm/dm_plugins.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
11 changes: 1 addition & 10 deletions src/libdm/fb_generic.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions src/libdm/include/calltable.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
4 changes: 2 additions & 2 deletions src/libdm/qtgl/fb-qtgl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
4 changes: 2 additions & 2 deletions src/libdm/swrast/fb-swrast.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down

0 comments on commit 046f47b

Please sign in to comment.