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

[Cyril Bur <cyril.bur@au1.ibm.com>]

On Tue, 24 May 2016 22:33:22 +0930
Andrew Jeffery <andrew@aj.id.au> wrote:

> 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:
> 

Well well well, found it! I'll hurry long the series which solves anyway.

Thanks!!

> 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