search

jeremyevans / aqualung

Advanced music player
http://aqualung.jeremyevans.net
GNU General Public License v2.0
49 stars 15 forks source link

Use bounded string functions #15

Closed atsampson closed 4 years ago

atsampson commented 4 years ago

This is the other set of changes I had sitting around from a couple of years ago - I've rebased it against the current version and split it up into (hopefully) reviewable chunks, but it's still pretty big, I'm afraid.

The general idea is to modernise Aqualung's string handling, using bounded operations like g_strlcpy, g_strlcat and snprintf instead of strcat etc. Since most of Aqualung uses fixed-size arrays for strings, the first commit introduces a handful of macros that let us (safely) avoid writing sizeof all over the place, and the majority of the changes were done mechanically, with a few functions needing to be modified to know the size of the strings they're writing to.

Overall the code ends up a bit simpler in most cases, and I've fixed several places where bounds checking was missing or wrong, so it should be a bit more resilient.

The one function I haven't modernised is file_transform - because I'm not entirely convinced I understand everything it's doing! It may be easier to rewrite it using GString in the future as I've done for metadata_value_all.

jeremyevans commented 4 years ago

Thanks for working on this. I agree in principle on switching to bounded string functions. I'll work on reviewing and testing this.

jeremyevans commented 4 years ago

This causes memory issues in the lua support when trying to play a track. This is on OpenBSD, which has a malloc system that is stricter than most.

aqualung(16289) in free(): modified chunk-pointer 0x8c67c8d2f60
Thread 1 received signal SIGABRT, Aborted.

(gdb) bt
#0  thrkill () at -:3
#1  0x000008c5e183ae6e in _libc_abort () at /usr/src/lib/libc/stdlib/abort.c:51
#2  0x000008c5e17dfd86 in wrterror (d=0x8c5b8528c70, msg=0x8c5e179b3de "modified chunk-pointer %p") at /usr/src/lib/libc/stdlib/malloc.c:300
#3  0x000008c5e17e310c in find_chunknum (d=0x0, info=<optimized out>, ptr=0x0, check=1) at /usr/src/lib/libc/stdlib/malloc.c:1067
#4  0x000008c5e17e0364 in ofree (argpool=0x7f7ffffef3e0, p=0x8c67c8d2f60, clear=0, check=<optimized out>, argsz=0)
    at /usr/src/lib/libc/stdlib/malloc.c:1431
#5  0x000008c5e17dffd0 in free (ptr=0x8c67c8d2f60) at /usr/src/lib/libc/stdlib/malloc.c:1488
#6  0x000008c38f3626ab in l_metadata_value (L=0x8c63f461c00) at ext_lua.c:233
#7  0x000008c6303555c2 in luaD_precall () from /usr/local/lib/liblua5.1.so.5.1
#8  0x000008c630363c4e in luaV_execute () from /usr/local/lib/liblua5.1.so.5.1
#9  0x000008c630355b9f in luaD_call () from /usr/local/lib/liblua5.1.so.5.1
#10 0x000008c630354e92 in luaD_rawrunprotected () from /usr/local/lib/liblua5.1.so.5.1
#11 0x000008c630355f31 in luaD_pcall () from /usr/local/lib/liblua5.1.so.5.1
#12 0x000008c63034f992 in lua_pcall () from /usr/local/lib/liblua5.1.so.5.1
#13 0x000008c38f362a38 in l_title_format (function_name=0x8c38f32e97a "application_title", fdec=0x8c5e00a7b00) at ext_lua.c:582
#14 0x000008c38f362b25 in application_title_format (fdec=0x8c5e00a7b00) at ext_lua.c:608
#15 0x000008c38f374326 in process_metablock (meta=0x8c5b3537160) at gui_main.c:3860
#16 0x000008c38f3738d8 in timeout_callback (data=<optimized out>) at gui_main.c:4081
#17 0x000008c38f3b7f9d in threads_dispatch (data=0x8c5daa91a20) at utils_gui.c:153
#18 0x000008c5f605e1c9 in ?? () from /usr/local/lib/libglib-2.0.so.4201.4
#19 0x000008c5f6062c78 in g_main_context_dispatch () from /usr/local/lib/libglib-2.0.so.4201.4
#20 0x000008c5f6063065 in ?? () from /usr/local/lib/libglib-2.0.so.4201.4
#21 0x000008c5f606347f in g_main_loop_run () from /usr/local/lib/libglib-2.0.so.4201.4
#22 0x000008c5e3aa2127 in gtk_main () from /usr/local/lib/libgtk-x11-2.0.so.2400.0
#23 0x000008c38f374599 in run_gui () at gui_main.c:4179
#24 0x000008c38f35fe77 in main (argc=1, argv=0x7f7fffff0ea8) at core.c:3183
jeremyevans commented 4 years ago

I found that reverting 5840203918972cef64f1c95cb0f8b36b54bf76af fixes the crash. Is it possible for you to rebase this against master with that commit removed? If not, I can probably get to it this weekend.

atsampson commented 4 years ago

I've rebased this on master without the metadata_value_all commit. I'll move that commit to a separate PR (I think I can see what's wrong with it...).

jeremyevans commented 4 years ago

Thanks for this. I'll do some testing with this over the next few days, and assuming no problems, I'll commit it.