[PATCH btbridge v4 1/6] Initialise variable to avoid using it uninitialised

[Andrew Jeffery]

On Thu, 2016-05-19 at 15:44 +1000, Cyril Bur wrote:
> > Building with this patch and native GCC* gives errors:
> > 
> >     $ KERNEL_HEADERS=../../linux/ast2400/include/uapi/ make
> >     cc  -Wall -O2 -g -I../../linux/ast2400/include/uapi/    btbridged.c  -lsystemd -o btbridged
> >     btbridged.c: In function ‘main’:
> >     btbridged.c:590:6: warning: ‘r’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> >        if (r < 0)
> >           ^
> >     btbridged.c:342:6: note: ‘r’ was declared here
> >       int r, len;
> >           ^
> > 
> > That's weird, because the note isn't relevant to the function of line
> > that generated the warning. However, the 'r' defined in bt_host_write()
> > suffers the same initialisation issue. Initialising it gives me a build
> > with no warnings so maybe it's worth doing that here also?
> > 
> 
> Yeah, GCC seems to be getting a bit confused. I'm usually not in favour of
> putting in stuff to shut the warnings up but these are just so odd that I'm at
> a loss.
> 
> Your suggestion works, lets just do that and forget about it.

So, just out of interest I ran scan-build over it. Turns out it leads
to a bug. Here's the reasoning:

339	static int bt_host_write(struct btbridged_context *context, struct bt_queue *bt_msg)
340	{
341		struct bt_queue *head;
342		uint8_t data[BT_MAX_MESSAGE] = { 0 };
343		sd_bus_message *msg = NULL;
344		int r, len;
	
1	'r' declared without an initial value	→
345	 
346		assert(context);
347	 
348		if (!bt_msg)
	
2←	Assuming 'bt_msg' is non-null	→
	
3←	Taking false branch	→
349			return -EINVAL;
350	 
351		head = bt_q_get_head(context);
352		if (bt_msg == head) {
	
4←	Assuming 'bt_msg' is not equal to 'head'	→
	
5←	Taking false branch	→
353			struct itimerspec ts;
354			/* Need to adjust the timer */
355			ts.it_interval.tv_sec = 0;
356			ts.it_interval.tv_nsec = 0;
357			if (head->next) {
358				ts.it_value = head->next->start;
359				ts.it_value.tv_sec += BT_HOST_TIMEOUT_SEC;
360				MSG_OUT("Adjusting timer for next element\n");
361			} else {
362				ts.it_value.tv_nsec = 0;
363				ts.it_value.tv_sec = 0;
364				MSG_OUT("Disabling timer, no elements remain in queue\n");
365			}
366			r = timerfd_settime(context->fds[TIMER_FD].fd, TFD_TIMER_ABSTIME, &ts, NULL);
367			if (r == -1)
368				MSG_ERR("Couldn't set timerfd\n");
369		}
370		data[1] = (bt_msg->rsp.netfn << 2) | (bt_msg->rsp.lun & 0x3);
371		data[2] = bt_msg->rsp.seq;
372		data[3] = bt_msg->rsp.cmd;
373		data[4] = bt_msg->rsp.cc;
374		if (bt_msg->rsp.data_len > sizeof(data) - 5) {
	
6←	Taking false branch	→
375			MSG_ERR("Response message size (%zu) too big, truncating\n", bt_msg->rsp.data_len);
376			bt_msg->rsp.data_len = sizeof(data) - 5;
377		}
378		/* netfn/len + seq + cmd + cc = 4 */
379		data[0] = bt_msg->rsp.data_len + 4;
380		if (bt_msg->rsp.data_len)
	
7←	Taking false branch	→
381			memcpy(data + 5, bt_msg->rsp.data, bt_msg->rsp.data_len);
382		/* Count the data[0] byte */
383		len = write(context->fds[BT_FD].fd, data, data[0] + 1);
384		if (len == -1) {
	
8←	Taking false branch     →
385			r = errno;
386			MSG_ERR("Error writing to %s: %s\n", BT_HOST_PATH, strerror(errno));
387			if (bt_msg->call) {
388				r = sd_bus_message_new_method_errno(bt_msg->call, &msg, r, NULL);
389				if (r < 0)
390					MSG_ERR("Couldn't create response error\n");
391			}
392			goto out;
393		} else {
394			if (len != data[0] + 1)
	
9←	Taking false branch	→
395				MSG_ERR("Possible short write to %s, desired len: %d, written len: %d\n", BT_HOST_PATH, data[0] + 1, len);
396			else
397				MSG_OUT("Successfully wrote %d of %d bytes to %s\n", len, data[0] + 1, BT_HOST_PATH);
398	 
399			if (bt_msg->call) {
	
10←	Taking false branch	→
400				r = sd_bus_message_new_method_return(bt_msg->call, &msg);
401				if (r < 0) {
402					MSG_ERR("Couldn't create response message\n");
403					goto out;
404				}
405				r = 0; /* Just to be explicit about what we're sending back */
406				r = sd_bus_message_append(msg, "x", r);
407				if (r < 0) {
408					MSG_ERR("Couldn't append result to response\n");
409					goto out;
410				}
411	 
412			}
413		}
414	 
415	out:
416		if (bt_msg->call) {
	
11←	Taking false branch	→
417			if (sd_bus_send(context->bus, msg, NULL) < 0)
418				MSG_ERR("Couldn't send response message\n");
419			sd_bus_message_unref(msg);
420		}
421		bt_q_drop(context, bt_msg);
422	 
423		/*
424		 * There isn't another message ready to be sent so turn off POLLOUT
425		 */
426		if (!bt_q_get_msg(context)) {
	
12←	Taking false branch	→
427			MSG_OUT("Turning off POLLOUT for the BT in poll()\n");
428			context->fds[BT_FD].events = POLLIN;
429		}
430	 
431		return r;
	
13←	Undefined or garbage value returned to caller
432	}
433
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160524/7891f83d/attachment.sig>