Re: Possible uninitialized value since commit 478ffaa7 ipmi-config: correct IPv6 dynamic address checkout error

[Pavel Cahyna]

[ please Cc: me in replies, I am not subscribed and my attempt to
subscribe hits "Bug in Mailman version 2.1.29" .]

Hello Al,

> certain fields are only used for static vs dynamic addresses, so only 
> specific ones need to be initialized as long as things are programmed 
> correctly.
>
> IIRC, the patch below fixed the mistake that I used "source" instead of 
> "source_type" for dynamic addresses. 

that's my point - the patch below initializes "source_type" and leaves
"source" uninitialized, but then it keeps using "ipv6_data.source". To
me, the patch seems incomplete and 
                                                  
get_dynamic_address_source_type_string (ipv6_data.source)) < 0)
looks like it should be changed to
                                                  
get_dynamic_address_source_type_string (ipv6_data.source_type)) < 0)

What do you think? If not, I don't see where does ipv6_data.source get
initialized.

Regards, Pavel

On Tue, Feb 20, 2024 at 05:03:46PM +0100, Pavel Cahyna wrote:
> Hello,
> 
> I have a question about this commit:
> 
> commit 478ffaa7d2ffe240a0afaf9d7d5189342c94bd4d
> Author: Albert Chu <chu11@llnl.gov>
> Date:   Wed Jan 26 21:48:47 2022 -0800
> 
>     ipmi-config: correct IPv6 dynamic address checkout error
> 
> 
> It contains this change:
> 
> -----
> @@ -1089,15 +1090,15 @@ _get_ipv6_dynamic_address (ipmi_config_state_data_t 
> *state_data,
>        goto cleanup;
>      }
> 
> -  if (FIID_OBJ_GET (obj_cmd_rs, "source", &val) < 0)
> +  if (FIID_OBJ_GET (obj_cmd_rs, "source_type", &val) < 0)
>      {
>        pstdout_fprintf (state_data->pstate,
>                         stderr,
> -                       "fiid_obj_get: 'source': %s\n",
> +                       "fiid_obj_get: 'source_type': %s\n",
>                         fiid_obj_errormsg (obj_cmd_rs));
>        goto cleanup;
>      }
> -  ipv6_data->source = val;
> +  ipv6_data->source_type = val;
> 
>    if (fiid_obj_get_data (obj_cmd_rs,
>                           "address",
> -----
> 
> 
> which, IIUC, leaves ipv6_data->source uninitialized.
> 
> It then contains this change:
> -----
> @@ -1186,7 +1187,7 @@ ipv6_dynamic_address_source_checkout 
> (ipmi_config_state_data_t *state_data,
> 
>    if (ipmi_config_section_update_keyvalue_output (state_data,
>                                                    kv,
> -                                                  
> get_dynamic_address_source_string (ipv6_data.source)) < 0)
> +                                                  
> get_dynamic_address_source_type_string (ipv6_data.source)) < 0)
>      return (IPMI_CONFIG_ERR_FATAL_ERROR);
> 
>    rv = IPMI_CONFIG_ERR_SUCCESS;
> -----
> 
> which keeps using ipv6_data.source, which looks uninitialized. This looks 
> suspect, but I admit I
> don't really understand the code.
> 
> Best regards, Pavel