Project

General

Profile

Bug #4418

svc.startd crash with a long instance name and more than one dependency

Added by Dan Vatca over 5 years ago. Updated about 4 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
cmd - userland programs
Start date:
2013-12-23
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
needs-triage

Description

We got this crash on svc.startd:

-----------------  lwp# 7 / thread# 7  --------------------
 feee57c5 _lwp_kill (7, 6, fe602b08, fef53000, fef53000, fe602b70) + 15
 fee80dcb raise    (6, 0, fe602b10, feea9440, 0, 0) + 2b
 fee5bd0e abort    (fe602b70, fe602b70, 34, 807aa4e, 807afde, a53) + 10e
 fee5c117 ???????? (807aa4e, 807afde, a53, 8112548)
 0806321c process_dependency_pg (8375588, fe602e34, 101, 0) + 3a8
 0806d33c walk_dependency_pgs (8103ca8, 8062e74, fe602e34, 1) + 143
 08062d1c set_dependencies (837a428, 8103ca8, fe602e7c, 8065b13, 837a428, 1) + 65
 0806507e refresh_vertex (837a428, 8103ca8, 1, 837a428, 0, fe602f4c) + e2
 08066776 configure_vertex (837a428, 8103ca8, fe602f58, 806cb66, 8103ca8, 8123a80) + a53
 0806699d dgraph_add_instance (8123a80, 8103ca8, 1, feeda2d3) + 1b8
 0806d0f5 libscf_populate_graph (8112548, fef53000, fe602fc8, 8067cf6) + 116
 08067dd2 graph_thread (0, 0, 0, 0) + e8
 feee0a1d _thrp_setup (fec52a40) + 88
 feee0bb0 _lwp_start (fec52a40, 0, 0, 0, 0, 0)

Steps to reproduce:

INST=a1234567890123456789012345678901234567890123456789012345678901234567789012345
svccfg -s svc:/system/filesystem/zfs/auto-snapshot add ${INST}
svcadm refresh svc:/system/filesystem/zfs/auto-snapshot:${INST}
svcadm enable svc:/system/filesystem/zfs/auto-snapshot:${INST}
pgrep svc.startd

Expected:
Either add instance or fail with "Invalid name".

Got:
Assertion failed: err == 0, file graph.c, line 2643

This tripped the assert after creating the depgroup vertex for property group. After some debugging, it turns out that the graph_insert_vertex_unconfigured returned EEXIST (in http://src.illumos.org/source/xref/illumos-gate/usr/src/cmd/svc/startd/graph.c#2643). Which is strange, because it shouldn't have.
The reason is that process_dependency_pg constructs an fmri-like string to hold both the fmri and the depended upon property group glued together by an '>' character (http://src.illumos.org/source/xref/illumos-gate/usr/src/cmd/svc/startd/graph.c#2591). This fmri-like string is then inserted as a vertex in the dependency graph (in http://src.illumos.org/source/xref/illumos-gate/usr/src/cmd/svc/startd/graph.c#2641). The issue is that the length of the string + the length of the depended on property group may be larger. Yet the snprintf that constructs it, limits the fmri-like string to max_scf_name_size, which seems wrong, as it is just one line after computing and allocating the fmri-like string. The truncation meant that it was possible to get a vertex named "svc:/loong...fmri>dep" to be truncated as "svc:/loong…fmr". And if the service had more than one dep, the attempt to add the second vertex would fail and trip the assert.

The fix should be simple:

diff --git a/usr/src/cmd/svc/startd/graph.c b/usr/src/cmd/svc/startd/graph.c
index 7fbf17a..7506e8e 100644
--- a/usr/src/cmd/svc/startd/graph.c
+++ b/usr/src/cmd/svc/startd/graph.c
@@ -2588,7 +2588,7 @@ process_dependency_pg(scf_propertygroup_t *pg, struct deppg_info *info)
        fmri_sz = strlen(info->v->gv_name) + 1 + len + 1;
        fmri = startd_alloc(fmri_sz);

-       (void) snprintf(fmri, max_scf_name_size, "%s>%s", info->v->gv_name,
+       (void) snprintf(fmri, fmri_sz, "%s>%s", info->v->gv_name,
            pg_name);

        /* Validate the pg before modifying the graph */

Now I am testing that it does not break other assumptions.

History

#1

Updated by Yuri Pankov over 4 years ago

Hi Dan, are you still working on this issue?

#2

Updated by Dan Vatca over 4 years ago

Yuri Pankov wrote:

Hi Dan, are you still working on this issue?

Hi Yuri,
I got this fix up for review: https://www.illumos.org/rb/r/38
It has been in our repository from the time I reported it, and was also used in production.

#3

Updated by Electric Monk about 4 years ago

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

git commit 09f79f7c66b85f056db11f58210dc6182c9b1aef

commit  09f79f7c66b85f056db11f58210dc6182c9b1aef
Author: Dan Vatca <dan@syneto.net>
Date:   2015-05-27T18:14:10.000Z

    4418 svc.startd crash with a long instance name and more than one dependency
    Reviewed by: Yuri Pankov <yuri.pankov@gmail.com>
    Reviewed by: Marcel Telka <marcel@telka.sk>
    Reviewed by: Albert Lee <trisk@omniti.com>
    Approved by: Richard Lowe <richlowe@richlowe.net>

Also available in: Atom PDF