On 06/29/10 08:08 AM, Matt Keenan wrote:
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


That's fine with me Matt.

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

Reply via email to