Hi Frank,

Thanks for reviewing my fix.  I'll respond to your questions below.

On 12/22/09 05:55 AM, Frank Batschulat (Home) wrote:
On Sat, 19 Dec 2009 04:28:52 +0100, Jordan Vaughan <jordan.vaug...@sun.com> 

I expanded my webrev to include my fix for

6910339 zonecfg coredumps with badly formed 'select net defrouter'

I need someone to review my changes.  The webrev is still accessible via


Hey Jordan looks good to me modulo this in zonecfg_lookup_nwif()

         size_t addrspec;                /* nonzero if tabptr has IP addr */
         size_t physspec;                /* nonzero if tabptr has interface */
+        size_t defrouterspec;           /* nonzero if tabptr has def. router */
if (tabptr == NULL)
                 return (Z_INVAL);
+ * zone_nwif_address, zone_nwif_physical, and zone_nwif_defrouter are
+         * arrays, so no NULL checks are necessary.
         addrspec = strlen(tabptr->zone_nwif_address);
         physspec = strlen(tabptr->zone_nwif_physical);
-        assert(addrspec > 0 || physspec > 0);
+        defrouterspec = strlen(tabptr->zone_nwif_defrouter);
+        assert(addrspec != 0 || physspec != 0 || defrouterspec != 0);
so we do consider any of them being 0 a fault given the assert(), fine, but yet
we do check for this again inside the loop:

+                if (physspec != 0 && (fetchprop(cur, DTD_ATTR_PHYSICAL,
+                    physical, sizeof (physical)) != Z_OK ||
+                    strcmp(tabptr->zone_nwif_physical, physical) != 0))
+                        continue;
+                if (addrspec != 0 && (fetchprop(cur, DTD_ATTR_ADDRESS, address,
+                    sizeof (address)) != Z_OK ||
+                    !zonecfg_same_net_address(tabptr->zone_nwif_address,
+                    address)))
+                        continue;
+                if (defrouterspec != 0 && (fetchprop(cur, DTD_ATTR_DEFROUTER,
+                    address, sizeof (address)) != Z_OK ||
+                    !zonecfg_same_net_address(tabptr->zone_nwif_defrouter,
+                    address)))
+                        continue;

a good argument could probably be made to turn this assert into a real
check and return Z_INVAL for any of those 3 being 0 and get rid of
the checks inside the xml parsing loop ?

The assertion doesn't fail if any of the three variables is zero; it fails if all of them are zero. However, your suggestion that we transform the assertion into a real check that returns Z_INVAL or Z_INSUFFICIENT_SPEC is good. I was able to easily produce a core dump on my system even without my fix:

root arrakis [16:12:49]# zonecfg -z mojo
zonecfg:mojo> select net address=""
Assertion failed: addrspec > 0 || physspec > 0, file ../common/libzonecfg.c, line 2170
zsh: IOT instruction (core dumped)  cz mojo

I verified that changing the assertion into a real check that returns Z_INSUFFICIENT_SPEC eliminates the problem:

root tcm3000-01 [16:13:03 1]# cz mojo
zonecfg:mojo> select net address=""
select net: Insufficient specification

However, the three checks in the loop (physspec != 0, etc.) are necessary even after converting the assertion into a non-asserting test. Suppose that a zone were to have the following net configuration:

zonecfg:mojo> info net
        physical: bge0
        defrouter not specified

If I were to eliminate the three checks in the loop, then if I were to issue a "select net address=", then zonecfg(1M) would claim that the zone doesn't have a network resource with an address of! This follows from the way the three if statements would work without the three aforementioned checks: physspec would be zero (because the query doesn't specify a physical interface) but the network resource's physical property would be nonempty, which would make the strcmp(3C) invocation in the first if statement return a nonzero value and cause the function to skip the network resource that it would have otherwise selected!

Here is some output from zonecfg(1M) while it's using a libzonecfg that lacks the three loop checks:

root tcm3000-01 [16:25:12 1]# cz mojo
zonecfg:mojo> info
zonename: mojo
zonepath: /export/mojo
brand: solaris10
autoboot: true
ip-type: shared
        physical: bge0
        defrouter not specified
zonecfg:mojo> select net address=
select net: No such resource with that id

zone_lookup_nwif() needs the three loop checks.

I regenerated the webrev. You'll notice that the assertion was replaced by a check that returns Z_INSUFFICIENT_SPEC.

Thanks again for reviewing my fix,


zones-discuss mailing list

Reply via email to