tvservice_notify_func: Conditional jump or move depends on uninitialised value(s)

[minfrin]

valgrind picked up the following:

==1574== Thread 7 HTV Notify:
==1574== Conditional jump or move depends on uninitialised value(s)
==1574== at 0x6BF0A64: tvservice_notify_func (in /opt/vc/lib/libbcm_host.so)
==1574== Uninitialised value was created by a stack allocation
==1574== at 0x6BF095C: tvservice_notify_func (in /opt/vc/lib/libbcm_host.so)

Looking at stack allocated variables in tvservice_notify_func in https://github.com/raspberrypi/userland/blob/master/interface/vmcs_host/vc_vchi_tvservice.c I tripped over the following code:

   TV_DISPLAY_STATE_T tvstate;

   vcos_log_trace("TV service async thread started");
   /* Check starting state, and put service in use if necessary */
   tvservice_send_command_reply( VC_TV_GET_DISPLAY_STATE, NULL, 0, &tvstate, sizeof(TV_DISPLAY_STATE_T));
   if (tvstate.state & VC_HDMI_ATTACHED)

tvstate will be left initialised if tvservice_send_command_reply() returns something other than success, leaving " if (tvstate.state & VC_HDMI_ATTACHED)" bogus.

tvservice_send_command_reply() needs to be error checked as is everywhere else in the code.

[popcornmix]

Does this fix look okay?
http://paste.ubuntu.com/24635138/

[minfrin]

It's not enough, no - the tvstate variable is used in a number of places inside the while loop below this code.

The case when tvservice_send_command_reply() returns something other than success needs to be properly catered for. I don't understand the code fully enough to say how.

[popcornmix]

Realistically this will never occur unless firmware is fatally broken.
As such bailing out of the notification task makes as much sense as anything, so lets do that.

Does this keep valgrind happy?
http://paste.ubuntu.com/24643145/

[popcornmix]

This fix has been committed. Can you test and close if happy?

[JamesH65]

Assuming happy, closing.