GhostNaN / mpvpaper

A video wallpaper program for wlroots based wayland compositors.
GNU General Public License v3.0
805 stars 26 forks source link

Memory error fixes, slight refactoring #23

Closed mstoeckl closed 2 years ago

mstoeckl commented 2 years ago

After building mpvpaper with the -Db_sanitize=address flag for meson, I found an ~uninitialized memory~ null pointer issue and an out-of-bounds access when starting the program; see attached commits for the fixes. The first commit does a bit of linked refactoring to make the way the options are transferred clearer; let me know if you'd prefer a minimal change that fixes the issue.

GhostNaN commented 2 years ago

Odd... I didn't get any errors like you describe. This is what I added to meson.build: add_project_arguments('-Db_sanitize=address', '-O1', '-Wp,-D_FORTIFY_SOURCE=2', '-Wformat', '-Werror=format-security', '-fstack-clash-protection', '-fcf-protection', language : 'c')

mstoeckl commented 2 years ago

-Db_sanitize=address is not a C language flag, but an option passed to meson when setting up the build, like meson mpvpaper build-dir -Db_sanitize=address. The equivalent C compiler flag is -fsanitize=address, which works for both clang and gcc.

GhostNaN commented 2 years ago

I still can't replicate the issue. I looked over the code anyways. I totally forgot to free mpv_options my bad, didn't realize I needed to after a strdup(optarg) The fix doesn't have to be that complicated though. As seen here at line 942:

 fputs(mpv_options, file);
 free(mpv_options);
 fclose(file);

I think I had a stoke, let me look through that again.

GhostNaN commented 2 years ago

Ok, I'm ok with the changes you did with moving the "mpv options into a config file" code. It is more transparent as to where the options are coming from anyways. But there's no point in putting mpv_options in the wl_state struct. Just make it a global and replace static char *opt_config_path; with it at line 61. Like so : static char *mpv_options; That's just my personal preference.

As for the second commit, Replacing a bool with a int is not a big deal so I'm cool with that.

Make those changes and I think we are good to go.

mstoeckl commented 2 years ago

But there's no point in putting mpv_options in the wl_state struct. Just make it a global and replace static char opt_config_path; with it at line 61. Like so : static char mpv_options;

Updated.

GhostNaN commented 2 years ago

Thank You for your contribution!