From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: References: Date: Mon, 20 May 2013 13:18:38 -0700 Message-ID: Subject: Re: Does sdp_seq_alloc_with_length() invoke undefined behavior? From: Luiz Augusto von Dentz To: Michael Bradshaw Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Michael, On Mon, May 20, 2013 at 10:36 AM, Michael Bradshaw wrote: > After reading some sample HID client code[1] that brought up the > suspicious code in sdp_seq_alloc_with_length(), I'm wondering if it > possibly invokes undefined behavior. > > I'll annotate the function: > sdp_data_t *sdp_seq_alloc_with_length(void **dtds, void **values, int *length, > int len) > { > sdp_data_t *curr = NULL, *seq = NULL; > int i; > > for (i = 0; i < len; i++) { > // ... removed to be concise ... > } > > return sdp_data_alloc_with_length(SDP_SEQ8, seq, length[i]); > } > > That last line looks like it is accessing one-past-the-end of the > length array when it says length[i]. Should the code execute that > line, i == len, and if len represents the number of elements in the > length array (which I think it does, but correct me if it does not), > then it does indeed invoke undefined behavior. This might actually be bug, the function is not that much that is probably why we haven't notice this problem before. Anyway it is leaking whenever it fails because we don't free the data allocated previously. > I'm hoping I could get some input from bluez developers who can say > with confidence whether or not this is a bug (so I know whether or not > I should open a bug report). If you have the backtrace it is usually easier for us to tell but just by looking at the code it seems very problematic. -- Luiz Augusto von Dentz