On 06/28/10 06:18 PM, [email protected] wrote:
On 06/28/10 12:16 PM, Matt Keenan wrote:
Thanks *Joe*, and John for your review.

I've updated webrev relating to both sets of comments.

cheers

Matt


On 06/28/10 04:58 PM, John Fischer wrote:
Opps I meant Joe.

John

On 06/28/10 08:53 AM, John Fischer wrote:
Matt,

In addition to what Jack said about doing the nvlist_free() and assigning
NULL right after, something else doesn't seem right in the code.

We have:

1205 if (nvlist_alloc(&ti_ex_attrs, TI_TARGET_NVLIST_TYPE, 0) != 0) {


Then we do some stuff with it and then free it at:

1228         nvlist_free(ti_ex_attrs);

Then latter on:

1246         if (prepare_zfs_root_pool_attrs(&ti_ex_attrs, disk_name,
1247             install_slice_id) != OM_SUCCESS) {
1248 om_log_print("Could not prepare ZFS root pool attribute set\n");
1249                 nvlist_free(ti_ex_attrs);
1250                 status = -1;
1251                 goto ti_error;
1252         }


prepare_zfs_root_pool_attrs() calls nvlist_alloc() and can return something
other then OM_SUCCESS if nvlist_alloc() fails:

2542         if (nvlist_alloc(attrs, TI_TARGET_NVLIST_TYPE, 0) != 0) {
2543 om_log_print("Could not create target nvlist.\n");
2544
2545                 return (OM_FAILURE);
2546         }


Thus ti_ex_attrs has not been allocated and free-ing it at line 1249 is in
error as well.

prepare_volume_attrs() has a similar situation happening which is what
you have fixed with the NULL assignment and the checks for NULL before
free-ing.

prepare_be_attrs() ditto:

1424         if (prepare_be_attrs(&ti_ex_attrs) != OM_SUCCESS) {
1425 om_log_print("Could not prepare BE attribute set\n");
1426                 nvlist_free(ti_ex_attrs);
1427                 status = -1;
1428                 goto ti_error;
1429         }

So there appears to be other places within this code that the NULL assignment and
checks need to be done.

Thanks,

John

On 06/28/10 05:56 AM, Matt Keenan wrote:
Quick Code review request for :
Bug 16192 - auto-install core dumped when creating invalid size swap or dump
  http://defect.opensolaris.org/bz/show_bug.cgi?id=16192

Webrev :
  http://cr.opensolaris.org/~mattman/bug-16192/


Whilst auto-install does core, it does not have any adverse affect on the install as the install is in the process of failing/quitting either way.

Problem occurs because nvlist_free() is being called on an nvlist variable that was never allocated.

Solution is to assign NULL to nvlist variable prior to possible allocation. If it gets allocated it will be non-null and thus can be freed.

cheers

Matt
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss

_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss

_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss

_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss

Hey Matt,

Question/Suggestion:

Do you want to set it to NULL after these invocations to nvlist_free(ti_ex_attrs) ?

1213                 nvlist_free(ti_ex_attrs);
1250                         nvlist_free(ti_ex_attrs);
1349                                         nvlist_free(ti_ex_attrs);
1386                                 nvlist_free(ti_ex_attrs);
1430                         nvlist_free(ti_ex_attrs);

I realize all of these are followed with a call to goto ti_error; but future code changes could be problematic if ti_ex_attrs is not set to NULL after freeing.


To be honest I think that would be just overkill, and as liborchestrator is in time being made obsolete, future code changes should be minimal.

cheers

Matt

Your call.



Joe

_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss

Reply via email to