Project

General

Profile

Actions

Bug #14136

closed

acctadm *SOMETIMES* triggers smatch error

Added by Dan McDonald about 2 months ago. Updated 16 days ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
-
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:

Description

It's been more frequent of late in our Jenkins builds. We're seeing:

/root/data/jenkins/workspace/smartos-master-debug/projects/illumos/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: main.c:377 main() warn: passing freed memory 'buf'
/root/data/jenkins/workspace/smartos-master-debug/projects/illumos/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: main.c:388 main() warn: passing freed memory 'buf'

Even though the ONLY way "buf" is freed before hitting those two lines are via free() calls before die(), which exits the program. Is it a bug in the program? Or in smatch?

Actions #1

Updated by Jason King about 2 months ago

What happens if you label die() as __NORETURN in utils.h?

Actions #2

Updated by Dan McDonald about 2 months ago

Jason King wrote in #note-1:

What happens if you label die() as __NORETURN in utils.h?

Like this?

diff --git a/usr/src/cmd/acctadm/utils.h b/usr/src/cmd/acctadm/utils.h
index e40fa53f01..7a58cbf855 100644
--- a/usr/src/cmd/acctadm/utils.h
+++ b/usr/src/cmd/acctadm/utils.h
@@ -41,7 +41,7 @@ extern "C" {
 extern dladm_handle_t dld_handle;

 extern void warn(const char *, ...);
-extern void die(char *, ...);
+extern void die(char *, ...) __NORETURN;
 extern char *setpname(char *);
 extern const char *ac_type_name(int);
 extern int open_exacct_file(const char *, int);

Seems to build okay. The BIG test would be to put it in and see if we still see that sort of failure in our Jenkins runs, but it's highly random, so it may take several tries. Plus we also see jenkins failures thanks to #14132 .

Actions #3

Updated by Dan McDonald 16 days ago

Testing notes:

Disassembly diffs show a near-100-instruction shrinkage in the post-this-fix acctadm binary. An example disassembly diff shows one instance of instruction reduction:

OLD:

-    aconf_init+0x129: e8 ad 10 00 00     call   +0x10ad        <die>
***-    aconf_init+0x12e: 83 c4 10           addl   $0x10,%esp
***-    aconf_init+0x131: e9 ed fe ff ff     jmp    -0x113 <aconf_init+0x23>
-    aconf_init+0x136: 83 ec 0c           subl   $0xc,%esp
-    aconf_init+0x139: 56                 pushl  %esi
-    aconf_init+0x13a: e8 6f 11 00 00     call   +0x116f        <ac_type_name>

NEW:
+    aconf_init+0x129: e8 65 10 00 00     call   +0x1065        <die>
+    aconf_init+0x12e: 83 ec 0c           subl   $0xc,%esp
+    aconf_init+0x131: 56                 pushl  %esi
+    aconf_init+0x132: e8 2f 11 00 00     call   +0x112f        <ac_type_name>

Two lines (indicated by ***) that presumably continue on in function flow are no longer there, thanks to the __NORETURN nature of die().

The disassembly output shows it's not two instructions per every call to die():

WS-nowhere-WS(cmd/acctadm)[1]% dis ./acctadm | wc -l
   4324
WS-nowhere-WS(cmd/acctadm)[0]% dis ./acctadm | grep die | wc -l
    105
WS-nowhere-WS(cmd/acctadm)[0]% dis ./acctadm.fixed  | wc -l
   4235
WS-nowhere-WS(cmd/acctadm)[0]% dis ./acctadm.fixed | grep die | wc -l
    105
WS-nowhere-WS(cmd/acctadm)[0]% bc
4324 - 4235
89
WS-nowhere-WS(cmd/acctadm)[0]% 

And finally, exercising a couple of die() paths:


nowhere(~)[1]% acctadm -e flow task
acctadm: insufficient authorization to change task extended accounting configuration
nowhere(~)[1]% build/illumos-gate/proto/root_i386/usr/sbin/acctadm -e flow task
acctadm: insufficient authorization to change task extended accounting configuration
nowhere(~)[1]% acctadm -s
acctadm: -s option should only be invoked by smf(5)
nowhere(~)[1]% build/illumos-gate/proto/root_i386/usr/sbin/acctadm -s
acctadm: -s option should only be invoked by smf(5)
nowhere(~)[1]% 
Actions #4

Updated by Electric Monk 16 days ago

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

git commit 1dee82f4f45d5f7651c78ce447f9829386d9d9c5

commit  1dee82f4f45d5f7651c78ce447f9829386d9d9c5
Author: Dan McDonald <danmcd@joyent.com>
Date:   2021-11-11T20:45:31.000Z

    14136 acctadm *SOMETIMES* triggers smatch error
    Reviewed by: Mike Zeller <mike.zeller@joyent.com>
    Reviewed by: Brian Bennett <brian.bennett@joyent.com>
    Approved by: Robert Mustacchi <rm@fingolfin.org>

Actions

Also available in: Atom PDF