On 7/30/20 11:02 AM, Colin Ian King wrote:
> Hi,
>
> Static analysis with Coverity has detected an uninitialized value being
> used in a comparison. The error was detected on a recent change to
> drivers/staging/greybus/audio_topology.c however the issue actually
> dates back to the original commit:
>
> commit 6339d2322c47f4b8ebabf9daf0130328ed72648b
> Author: Vaibhav Agarwal
> Date: Wed Jan 13 14:07:51 2016 -0700
>
> greybus: audio: Add topology parser for GB codec
>
> The analysis is as follows:
>
> 425 static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol,
> 426 struct snd_ctl_elem_value
> *ucontrol)
> 427 {
> 428int ret, wi, max, connect;
> 429unsigned int mask, val;
> 430struct gb_audio_ctl_elem_info *info;
> 431struct gbaudio_ctl_pvt *data;
>
>1. var_decl: Declaring variable gbvalue without initializer.
> 432struct gb_audio_ctl_elem_value gbvalue;
> 433struct gbaudio_module_info *module;
> 434struct snd_soc_dapm_widget_list *wlist =
> snd_kcontrol_chip(kcontrol);
> 435struct snd_soc_dapm_widget *widget = wlist->widgets[0];
> 436struct device *codec_dev = widget->dapm->dev;
> 437struct gbaudio_codec_info *gb = dev_get_drvdata(codec_dev);
> 438struct gb_bundle *bundle;
> 439
>
>2. Condition 0 /* __builtin_types_compatible_p() */, taking false branch.
>3. Condition 1 /* __builtin_types_compatible_p() */, taking true branch.
>4. Falling through to end of if statement.
>5. Condition !!branch, taking false branch.
>6. Condition ({...; !!branch;}), taking false branch.
>
> 440dev_dbg(codec_dev, "Entered %s:%s\n", __func__,
> kcontrol->id.name);
> 441module = find_gb_module(gb, kcontrol->id.name);
>
>7. Condition !module, taking false branch.
> 442if (!module)
> 443return -EINVAL;
> 444
> 445data = (struct gbaudio_ctl_pvt *)kcontrol->private_value;
> 446info = (struct gb_audio_ctl_elem_info *)data->info;
>
>8. Condition 0 /* !!(!__builtin_types_compatible_p() &&
> !__builtin_types_compatible_p()) */, taking false branch.
> 447bundle = to_gb_bundle(module->dev);
> 448
>
>9. Condition data->vcount == 2, taking true branch.
> 449if (data->vcount == 2)
> 450dev_warn(widget->dapm->dev,
> 451 "GB: Control '%s' is stereo, which is not
> supported\n",
> 452 kcontrol->id.name);
> 453
> 454max = le32_to_cpu(info->value.integer.max);
> 455mask = (1 << fls(max)) - 1;
> 456val = ucontrol->value.integer.value[0] & mask;
>
>10. Condition !!val, taking true branch.
> 457connect = !!val;
> 458
> 459/* update ucontrol */
>
> Uninitialized scalar variable (UNINIT)
>11. uninit_use: Using uninitialized value gbvalue.value.integer_value[0].
> 460if (gbvalue.value.integer_value[0] != val) {
>
> The gbvalue.value.integer_value[0] read is bogus since gbvalue was
> declared on the stack but was not initialized. There seems to be no
> where that sets this data. I'm assuming most of the time that the
> comparison works because the garbage value is different from val and so
> the code in the if stanza is executed.
>
> Anyhow, I'm unsure what the original intent of the code was, so I've not
> attempted to fix this.
I think the fix is to add a call to this:
ret = gb_audio_gb_get_control(module->mgmt_connection, data->ctl_id,
GB_AUDIO_INVALID_INDEX, );
before the field within gbvalue is used.
Looking at gbcodec_mixer_dapm_ctl_get() defined just above that, it
seems that the call to gb_audio_gb_get_control() should be preceded
by a call to gb_pm_runtime_get_sync(). And given that duplication,
I suggest this call and the PM runtime wrapper functions should be
placed in a new helper function.
I know that Vaibhav said he would be fixing this, so I guess my
comments are directed at him. Thanks for sending the patch Colin.
-Alex
> Colin
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
