Segfault added in the recent changes

[Sebastien Bacher]

[Impact]

On hirsute/impish, update-notifier can crash when attempting to run hooks.
This issue is the #2 crash on update-notifier in hirsute, presently (and
maybe some of the others). I would call this a severe regression.

In the crash cases, cleanup steps are running on an uninitialized variable. The fix resolves this by ensuring the relevant variable is initialized.

[Test Case]

Create a file 'bug'

'Name: bug
Priority: Low
Command: "/bin/true"
DisplayIf: /bin/true
Description: update notifier bug'

and copy it to /var/lib/update-notifier/user.d

[Where problems could occur]

This is not the most absolute minimal version of this fix.
If the minimal version of the fix is desired, I can provide that, which
should be a +2/-2 diff. (I don't want to confuse the current proposal.)
The fix changeset being proposed includes other glib usage improvements
which sound really good from a maintainability perspective and may even
close other yet unknown bugs but add their own risk of regressions if
said maintainability was not done correctly.

The original idea was to address a problem where we offer to run a
command, and the user would click the button to do so, but then nothing
would happen because the command wasn't on the system. If the parsing
of this command is invalid, then valid hooks may be filtered.

It's possible for the command to contain arguments, and to be quoted (or
not), or to not be the full command path. Variables for such testing
include:
* rely on path lookup or fully qualified path supplied in command
* quoted or unquoted or mixed quotes (quote the command and args
  outside of the quotes)
* arguments or no
* presence or absence of actual command
See attached tarball for the resulting 20 tests (4 redundant tests pruned).
All the 'present' hooks are valid, 'absent' hooks should be skipped.

In an attempt to stave off further memory safety problems I've analyzed
this version with valgrind. valgrind is able to observe several
problems in the unfixed version, whereas the fixed version only
complains about some items in glib that appear to be unrelated to this
change.

I've got some unit test code as well but haven't integrated that yet to
the update-notifier codebase.

[Original Bug Report Description]

The issue is new using
https://code.launchpad.net/~dbungert/update-notifier/+git/update-notifier/+merge/397367
, the previous revision didn't have the issue

* Create a file 'bug'

'Name: bug
Priority: Low
Command: "/bin/true"
DisplayIf: /bin/true
Description: update notifier bug'

and copy it to /var/lib/update-notifier/user.d

-> update-notifier segfaults

(gdb) bt
#0 0x00007ffff718c769 in __GI___libc_free (mem=0x5550000aa7f9) at malloc.c:3288
#1 0x00007ffff7365215 in g_strfreev (str_array=<optimized out>) at ../../../glib/gstrfuncs.c:2553
#2 g_strfreev (str_array=0x5555555d9e00) at ../../../glib/gstrfuncs.c:2546
#3 0x000055555555dd2d in hook_command_exists (cmd=0x555555702600 "\"/usr/bin/eog\"") at hooks.c:137
#4 0x000055555555fd6e in is_hook_relevant (hook_file=0x555555829e7b "apt-file.update-notifier")
    at hooks.c:717
#5 0x000055555555ffb1 in check_update_hooks (ta=0x5555556c9a20) at hooks.c:781
#6 0x0000555555560715 in hook_tray_icon_init (ta=0x5555556c9a20) at hooks.c:969
#7 0x000055555555cd77 in tray_icons_init (un=0x55555562c3e0,

It seems likely to be the error on top of the weekly report for hirsute
but that's missing a stacktrace

One comment in addition on the change there, was the env split + iterate
over pathdirs basically a re-implementation of
https://developer.gnome.org/glib/stable/glib-Miscellaneous-Utility-Functions.html#g-find-program-in-path
? If there is particular reason to do that I would recommend just using
the glib function instead (which might also fix the issue as a side
effect, I didn't check)

[Dan Bungert (dbungert)]

Marking confirmed as I can see a segfault, but mine looks a little different:

Thread 1 "update-notifier" received signal SIGSEGV, Segmentation fault.
show_next_hook (hooks=0x555555778340 = {...}, ta=<optimized out>, ta=<optimized out>) at hooks.c:376
376 gtk_widget_hide(priv->button_next);
(gdb) bt
#0 show_next_hook (hooks=0x555555778340 = {...}, ta=0x5555556c9000, ta=<optimized out>) at hooks.c:376
#1 0x000055555555efdc in show_hooks (focus_on_map=0, ta=0x5555556c9000) at hooks.c:609
#2 check_update_hooks (ta=0x5555556c9000) at hooks.c:858
#3 0x000055555555f268 in hook_tray_icon_init (ta=<optimized out>) at hooks.c:969
#4 0x000055555555c933 in tray_icons_init (un=0x5555556e4f70) at update-notifier.c:447
#5 0x00007ffff7351e38 in () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#6 0x00007ffff735174f in g_main_context_dispatch () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#7 0x00007ffff73a4c68 in () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#8 0x00007ffff7350db3 in g_main_loop_run () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#9 0x00007ffff7a4517d in gtk_main () at /lib/x86_64-linux-gnu/libgtk-3.so.0
#10 0x000055555555c0f5 in main (argc=<optimized out>, argv=<optimized out>) at update-notifier.c:649

I like your suggestion to use g_find_program_in_path and plan to use that if at all possible.

[Dan Bungert (dbungert)]

Root cause:
The goto out for cleanup can skip a variable initialization, so in some cases we
crash on if(pathdirs) g_strfreev(pathdirs); because pathdirs is declared too
late and, in that code flow, never initialized.

We could thus also fix this by ensuring that pathdirs is properly initialized
at the beginning of the function, but removing the reimplementation of
g_find_program_in_path is the superior solution.

Attached is my proposed fix, however I don't think we should upload yet since there appear to be some parallel work going on in https://bugs.launchpad.net/ubuntu/+source/update-notifier/+bug/1883315

[Sebastien Bacher (seb128)]

Thanks Dan, the explanation of the issue and simplification as a solution are making sense, I've tested locally the patch and confirmed it resolves the segfault. The other bug you mention has its fix uploaded so nothing is preventing us to queue another upload now

[Dan Bungert (dbungert)]

MP: https://code.launchpad.net/~dbungert/update-notifier/+git/update-notifier/+merge/401901

[Ubuntu Foundations Team Bug Bot (crichton)]

The attachment "update-notifier-1-3.192.41.debdiff" seems to be a debdiff. The ubuntu-sponsors team has been subscribed to the bug report so that they can review and hopefully sponsor the debdiff. If the attachment isn't a patch, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are member of the ~ubuntu-sponsors, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issue please contact him.]

[Dan Bungert (dbungert)]

It is a patch but I'm not looking for a sponsor at this time, since it's going thru the MP.

[Brian Murray (brian-murray)]

I manually retraced the crash file Dan provided and the results follow:

Stacktrace:
 #0 0x00007f3250972202 in g_strfreev (str_array=<optimized out>) at ../../../glib/gstrfuncs.c:2552
         i = 0
 #1 g_strfreev (str_array=0x69726f6972500a67) at ../../../glib/gstrfuncs.c:2546
         i = <optimized out>
 #2 0x0000562c680f57ff in hook_command_exists (cmd=<optimized out>) at hooks.c:137
         cargv = 0x562c6a3edc70
         result = 1
         unquoted = 0x562c6a3cfb50 "/bin/true"
         error = 0x0
         cargc = 1
         pathdirs = 0x69726f6972500a67
         result = <optimized out>
         unquoted = <optimized out>
         cargv = <optimized out>
         error = <optimized out>
         out = <optimized out>
         cargc = <optimized out>
         pathdirs = <optimized out>
         i = <optimized out>
         pathdir = <optimized out>
         fname = <optimized out>
 #3 is_hook_relevant (hook_file=hook_file@entry=0x562c6a4c0073 "bug") at hooks.c:717
         res = <optimized out>
         filename = 0x562c6a2277e0 "/var/lib/update-notifier/user.d/bug"
         f = <optimized out>
         rfc822 = 0x562c6a19d540
         b = <optimized out>
 #4 0x0000562c680f6b65 in check_update_hooks (ta=0x562c6a40e1b0) at hooks.c:781
         new_mtime = <optimized out>
         elm = <optimized out>
         hf = <optimized out>
         dir = 0x562c6a185660
         hook_file = 0x562c6a4c0073 "bug"
         priv = 0x562c6a238bc0
         unseen_count = 0
         __PRETTY_FUNCTION__ = "check_update_hooks"
 #5 0x0000562c680f7268 in hook_tray_icon_init (ta=<optimized out>) at hooks.c:969
         priv = <optimized out>
 #6 0x0000562c680f4933 in tray_icons_init (un=0x562c6a1ddd30, un@entry=<error reading variable: value has been optimized out>) at update-notifier.c:447
 No locals.
 #7 0x00007f3250956e38 in g_timeout_dispatch (source=0x562c6a195330, callback=<optimized out>, user_data=<optimized out>) at ../../../glib/gmain.c:4889
         timeout_source = 0x562c6a195330
         again = <optimized out>
 #8 0x00007f325095674f in g_main_dispatch (context=0x562c6a0d7060) at ../../../glib/gmain.c:3337
         dispatch = 0x7f3250956e20 <g_timeout_dispatch>
         prev_source = 0x0
         begin_time_nsec = 0
         was_in_call = 0
         user_data = 0x562c6a1ddd30
         callback = 0x562c680f4870 <tray_icons_init>
         cb_funcs = <optimized out>
         cb_data = 0x562c6a1bedc0
         need_destroy = <optimized out>
         source = 0x562c6a195330
         current = 0x562c6a1317c0
         i = 0
         current = <optimized out>
         i = <optimized out>
         __func__ = {<optimized out> <repeats 16 times>}
         source = <optimized out>
         _g_boolean_var_ = <optimized out>
         was_in_call = <optimized out>
         user_data = <optimized out>
         callback = <optimized out>
         cb_funcs = <optimized out>
         cb_data = <optimized out>
         need_destroy = <optimized out>
         dispatch = <optimized out>
         prev_source = <optimized out>
         begin_time_nsec = <optimized out>
         _g_boolean_var_ = <optimized out>
 #9 g_main_context_dispatch (context=0x562c6a0d7...

[Dan Bungert (dbungert)]

Test hooks to cover the cases introduced by the original change.

[Dan Bungert (dbungert)]

Requesting sponsorship for SRU of hirsute-update-notifier-2-3.192.40.2.debdiff

[Steve Langasek (vorlon)]

Hello Sebastien, or anyone else affected,

Accepted update-notifier into hirsute-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/update-notifier/3.192.40.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-hirsute to verification-done-hirsute. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-hirsute. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Ubuntu SRU Bot (ubuntu-sru-bot)]

All autopkgtests for the newly accepted update-notifier (3.192.40.2) for hirsute have finished running.
The following regressions have been reported in tests triggered by the package:

update-motd/3.7 (ppc64el, arm64, s390x, armhf, amd64)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/hirsute/update_excuses.html#update-notifier

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

[Dan Bungert (dbungert)]

I believe update-notifier is the innocent bystander here, and the failure is caused by https://bugs.launchpad.net/ubuntu/+source/update-motd/+bug/1926660

[Dan Bungert (dbungert)]

The same scenario occurs on impish, without update-notifier being involved:
https://autopkgtest.ubuntu.com/packages/u/update-motd/impish/amd64

[Launchpad Janitor (janitor)]

This bug was fixed in the package update-notifier - 3.192.42

---------------
update-notifier (3.192.42) impish; urgency=medium

  [ Dan Bungert ]
  * Address segfault regression in the 3.192.40 update (LP: #1926298) related
    to hook command path lookup

 -- Brian Murray <email address hidden> Thu, 29 Apr 2021 15:07:27 -0700

[Dan Bungert (dbungert)]

Self-verified as follows:

* use virtual machine manager to create a KVM with
  /srv/iso/hirsute/ubuntu-21.04-desktop-amd64.iso, 4 cpu/8G mem
* Running installer, choosing default options in the install wizard except to
  correct timezone to MDT. "ubuntu" user created.
* Install proceeds until reboot prompt, and I choose reboot.
* Next->Next->Next thru "welcome to ubuntu"
* Software updater offers updates, choose "Install now", which causes the
  update-notifier version to go to 3.192.40.1
* Acquire tests.tgz from bug
* kill update-notifier process
* copy all hooks from tests.tgz to /var/lib/update-notifier/user.d
* log out / log in / wait
* observe crash dialog (as expected)
* remove crash entry from /var/crash. We don't want to confuse the expected
  crash from ver 3.192.40 / 3.192.40.1 with a crash on 3.192.40.2
* if ~/.config/update-notifier/hooks_seen is present we would want to remove
  it, but it isn't present for me
* add hirsute-proposed to sources.list
* instll update-notifier 3.192.40.2 via 'sudo apt install update-notifier' * remove hirsute-proposed from sources.list * log out / log in / wait * expect to see 10 notifications from update-notifier and no crash

[Dan Bungert (dbungert)]

Also 3.192.42, which has the impish version of this fix, is not showing crashes for this problem:
https://errors.ubuntu.com/problem/74e558a18105a756eb6a4b6aaa7145b79471754a

[Dan Bungert (dbungert)]

Same for 3.192.40.2 in hirsute-proposed.

[Launchpad Janitor (janitor)]

This bug was fixed in the package update-notifier - 3.192.40.2

---------------
update-notifier (3.192.40.2) hirsute; urgency=medium

  [ Dan Bungert ]
  * Address segfault regression in the 3.192.40 update (LP: #1926298) related
    to hook command path lookup

 -- Brian Murray <email address hidden> Thu, 29 Apr 2021 14:45:05 -0700

[Łukasz Zemczak (sil2100)]

The verification of the Stable Release Update for update-notifier has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.