Project

General

Profile

Actions

Bug #4450

closed

pointless condfree in libfmnotify

Added by Garrett D'Amore over 8 years ago. Updated 6 months ago.

Status:
Closed
Priority:
Low
Category:
lib - userland libraries
Start date:
2014-01-10
Due date:
% Done:

100%

Estimated time:
Difficulty:
Bite-size
Tags:
Gerrit CR:

Description

In libfmnotify.c:

595static void
596condfree(void *buf)
597{
598 if (buf != NULL)
599 free(buf);
600}
601
602void
603nd_free_event_info(nd_ev_info_t *ev_info)
604{
605 condfree(ev_info->ei_severity);
606 condfree(ev_info->ei_descr);
607 condfree(ev_info->ei_diagcode);
608 condfree(ev_info->ei_url);
609 condfree(ev_info->ei_uuid);
610 condfree(ev_info->ei_fmri);
611 condfree(ev_info->ei_from_state);
612 condfree(ev_info->ei_to_state);
613 condfree(ev_info->ei_reason);
614 fmev_rele(ev_info->ei_ev);
615 free(ev_info);
616}

The use of condfree() is rather silly, since free already has the semantics that free(NULL) is a no-op. (The C standard requires this.)

This would be a nice simple integration for someone looking for a trivial bug to fix.


Files

pid-downstack.d (311 Bytes) pid-downstack.d Dan McDonald, 2021-11-18 07:31 PM
4450-after.txt (10.8 KB) 4450-after.txt Dan McDonald, 2021-11-18 07:31 PM
4450-before.txt (11.3 KB) 4450-before.txt Dan McDonald, 2021-11-18 07:31 PM
Actions #1

Updated by Dan McDonald 9 months ago

  • Assignee set to Dan McDonald
Actions #2

Updated by Dan McDonald 9 months ago

  • Assignee changed from Dan McDonald to Spencer Evans-Cole
Actions #3

Updated by Dan McDonald 6 months ago

Testing will be interesting, since most distros do not, by default, exercise libfmnotify:

Here's OmniOS bloody:

root@bloody:~# for a in `find /usr /bin /sbin -type f ` ; do ldd $a |& grep -q libfmnotify; if [[ $? == 0 ]]; then echo $a ; fi ; done
root@bloody:~# 

Here's a SmartOS compute node on Triton:

[root@larry (kebecloud) ~]# for a in `find /usr /bin /sbin -type f ` ; do ldd $a |& grep -q libfmnotify; if [[ $? == 0 ]]; then echo $a ; fi ; done
/usr/lib/fm/notify/smtp-notify
[root@larry (kebecloud) ~]# 

Luckily I do have the SmartOS node, and triggering smtp-notify is not hard. Since it's a daemon, using DTrace shouldn't be hard either.

Actions #4

Updated by Dan McDonald 6 months ago

Testing steps:

1.) Run "pid-downstack.d" (attached) on running stock smtp-notify binary, and induce a failure. Output attached at 4450-before.txt.

[root@larry (kebecloud) /zones/root]# dtrace/pid-downstack.d `pgrep smtp-notify` nd_free_event_info

2.) Disable smtp-notify, replace libfmnotify.so.1 with fixed one, confirm library is indeed fixed:

[root@larry (kebecloud) /zones/root]# nm /usr/lib/fm/libfmnotify.so.1 | grep condfree
[70]    |      8979|        42|FUNC |LOCL |0    |15     |condfree
[root@larry (kebecloud) /zones/root]# mount -F lofs /zones/root/libfmnotify.so.1 /usr/lib/fm/libfmnotify.so.1 
[root@larry (kebecloud) /zones/root]# nm /usr/lib/fm/libfmnotify.so.1 | grep condfree

3.) Re-enable smtp-notify, repeat step 1, and see there are no more condfree calls. Output attached as 4450-after.txt.

[root@larry (kebecloud) /zones/root]# wc -l 4450-*.txt
     210 4450-after.txt
     220 4450-before.txt
     430 total
[root@larry (kebecloud) /zones/root]# grep -l condfree 4450-*.txt
4450-before.txt
[root@larry (kebecloud) /zones/root]# 
Actions #5

Updated by Andy Fiddaman 6 months ago

Dan McDonald wrote in #note-3:

Testing will be interesting, since most distros do not, by default, exercise libfmnotify:

Here's OmniOS bloody:

Sounds like you're sorted, but on OmniOS the service/fault-management/smtp-notify is just an optional package, easily installed.

Actions #6

Updated by Electric Monk 6 months ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 705b6680745618ebbf67feb254ce9a62511084a5

commit  705b6680745618ebbf67feb254ce9a62511084a5
Author: Spencer Evans-Cole <spencer.ec09@gmail.com>
Date:   2021-11-19T19:19:34.000Z

    4450 pointless condfree in libfmnotify
    Reviewed by: Dan McDonald <danmcd@joyent.com>
    Reviewed by: Toomas Soome <tsoome@me.com>
    Reviewed by: Robert Mustacchi <rm@fingolfin.org>
    Approved by: Rich Lowe <richlowe@richlowe.net>

Actions

Also available in: Atom PDF