Bug #4418
closedsvc.startd crash with a long instance name and more than one dependency
100%
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.
Updated by Yuri Pankov about 7 years ago
Hi Dan, are you still working on this issue?
Updated by Dan Vatca about 7 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.
Updated by Electric Monk about 7 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>