Sweets / tiramisu

Desktop notifications, the UNIX way
MIT License
747 stars 19 forks source link

Segfaults when given app names of certain lengths #25

Closed wishfort36 closed 3 years ago

wishfort36 commented 3 years ago

Hi. I'm the maintainer of tiramisu on nixpkgs/NixOS, and found a bug when reviewing a PR to update the version of tiramisu currently in nixpkgs.

When running

$ while :; do notify-send -a $STR "hi"; done

where $STR is a string of lengths 2, 5, 7, 13, 18, 20, 22, or 29 (tested up to 30), tiramisu segfaults after a few notifications.

It looks like the contents of the strings don't matter, but I've tested it mostly with lots of "a"-s. I've reproduced this on two NixOS machines, and on one running Arch, all of whom have similar environments/configurations. I've bisected, and it turns out that this doesn't happen on commits before a425e43, and does happen on commits after a425e43. I haven't tested it on a425e43 because I couldn't get it to compile.

Sweets commented 3 years ago

How bizarre. As soon as I can, I’ll look into it, thanks for bringing it to my attention.

I’ll say, currently I’m in the process of completely rewriting tiramisu (or mostly anyways), so for the time being it may be best to use the commits before I started rewriting for a production environment where users wouldn’t expect these bugs.

That being the case, I do also need testing, so this is also quite welcome. Also, why exactly is the build failing for you?

Sweets commented 3 years ago

I was unable to reproduce this exact bug in my testing, I did however get a lot of memory related errors when using strings of 5 characters with the notify-send -a command. That being said, I didn't get a segfault but I did get other memory related errors, so I'm inclined to believe that commit 2b86b20 may fix this issue, as it fixes the other issues that I encountered.

Let me know.

wishfort36 commented 3 years ago

That being said, I didn't get a segfault but I did get other memory related errors

It turns out that the errors I got never were segfaults; they were memory related all along. Here are some sample error messages:

malloc(): memory corruption (fast)
zsh: abort (core dumped)  ./tiramisu
free(): invalid pointer
zsh: abort (core dumped)  ./tiramisu
corrupted size vs. prev_size
zsh: abort (core dumped)  ./tiramisu
realloc(): invalid pointer
zsh: abort (core dumped)  ./tiramisu

I guess that's what I get when I'm sleepy and debugging late at night :(

I'm inclined to believe that commit 2b86b20 may fix this issue, as it fixes the other issues that I encountered.

It seems to have fixed the bug. Thanks!

Also, why exactly is the build failing for you?

On Arch Linux (gcc 11.1.0):

$ git checkout a425e43
$ make
cc -Wall -Wno-unused-value -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -pthread -I/usr/include/libmount -I/usr/include/blkid  src/tiramisu.c src/output.c -lgio-2.0 -lgobject-2.0 -lglib-2.0   -o tiramisu
src/output.c: In function ‘output_notification’:
src/output.c:50:9: error: ‘print_json’ undeclared (first use in this function)
   50 |     if (print_json)
      |         ^~~~~~~~~~
src/output.c:50:9: note: each undeclared identifier is reported only once for each function it appears in
src/output.c: In function ‘default_output’:
src/output.c:137:19: error: ‘delimiter’ undeclared (first use in this function)
  137 |         app_name, delimiter, app_icon, delimiter, replaces_id,
      |                   ^~~~~~~~~
make: *** [Makefile:14: tiramisu] Error 1

Doing the same on NixOS 20.09 (gcc 9.3.0) gives the same errors, but it also complains about this line in src/output.c, and says error: format not a string literal and no format arguments [-Werror=format-security]. This is easily fixable, as the line can be replaced with printf("%s", [...]), which doesn't give the error.

I don't know the codebase of tiramisu that well (and I'm also no expert in glib either), so I have no idea where or how print_json and delimiter are defined. All I know is that the errors go away in the next commit (e1eaa5).

Sweets commented 3 years ago

If the cloned branch of tiramisu isn't up to date with HEAD, that may be why these errors are caused. src/output.c isn't used anymore, and is soon to be deleted once I finish the rewrite.

wishfort36 commented 3 years ago

It seems like output.c is built specifially on a425e43 because it's in the makefile, but is removed from the makefile in the next commit.

Sweets commented 3 years ago

Yeah, I’d forgotten to remove it before committing. I’m assuming now that everything is good now though and it builds for you?

wishfort36 commented 3 years ago

Yes, all is good. I haven't gotten any crashes after the new commit; it builds on all commits after a425e43 (of those I've tried), and there haven't been any problems with HEAD.