This patch removes some blanks spaces in sdp source code.
>>From a915f0ca83f4b1424eb986f7b4314a2ff09a56fc Mon Sep 17 00:00:00 2001
From: Santiago Carot Nemesio <[email protected]>
Date: Tue, 27 Apr 2010 13:56:29 +0200
Subject: [PATCH 1/3] Removed blank spaces
Signed-off-by: Santiago Carot Nemesio <[email protected]>
---
lib/sdp.c | 34 +++++++++++++++++-----------------
1 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/lib/sdp.c b/lib/sdp.c
index 7cf710b..ca16118 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -3254,7 +3254,7 @@ static int copy_cstate(uint8_t *pdata, int pdata_len, const sdp_cstate_t *cstate
}
/*
- * This is a service search request.
+ * This is a service search request.
*
* INPUT :
*
@@ -3417,7 +3417,7 @@ end:
}
/*
- * This is a service attribute request.
+ * This is a service attribute request.
*
* INPUT :
*
@@ -3438,7 +3438,7 @@ end:
*
* sdp_list_t *attrid
* Singly linked list containing attribute identifiers desired.
- * Every element is either a uint16_t(attrSpec = SDP_ATTR_REQ_INDIVIDUAL)
+ * Every element is either a uint16_t(attrSpec = SDP_ATTR_REQ_INDIVIDUAL)
* or a uint32_t(attrSpec=SDP_ATTR_REQ_RANGE)
*
* OUTPUT :
@@ -3448,7 +3448,7 @@ end:
* !0:
* The service record
*/
-sdp_record_t *sdp_service_attr_req(sdp_session_t *session, uint32_t handle,
+sdp_record_t *sdp_service_attr_req(sdp_session_t *session, uint32_t handle,
sdp_attrreq_type_t reqtype, const sdp_list_t *attrids)
{
uint32_t reqsize = 0, _reqsize;
@@ -3494,7 +3494,7 @@ sdp_record_t *sdp_service_attr_req(sdp_session_t *session, uint32_t handle,
pdata += sizeof(uint16_t);
// get attr seq PDU form
- seqlen = gen_attridseq_pdu(pdata, attrids,
+ seqlen = gen_attridseq_pdu(pdata, attrids,
reqtype == SDP_ATTR_REQ_INDIVIDUAL? SDP_UINT16 : SDP_UINT32);
if (seqlen == -1) {
errno = EINVAL;
@@ -3558,7 +3558,7 @@ sdp_record_t *sdp_service_attr_req(sdp_session_t *session, uint32_t handle,
SDPDBG("sdp_cstate_t length : %d\n", cstate_len);
/*
- * a split response: concatenate intermediate responses
+ * a split response: concatenate intermediate responses
* and the last one (which has cstate_len == 0)
*/
if (cstate_len > 0 || rsp_concat_buf.data_size != 0) {
@@ -3583,7 +3583,7 @@ sdp_record_t *sdp_service_attr_req(sdp_session_t *session, uint32_t handle,
}
rec = sdp_extract_pdu(pdata, pdata_len, &scanned);
}
-
+
end:
if (reqbuf)
free(reqbuf);
@@ -3676,7 +3676,7 @@ int sdp_set_notify(sdp_session_t *session, sdp_callback_t *func, void *udata)
/*
* This function starts an asynchronous service search request.
- * The incomming and outgoing data are stored in the transaction structure
+ * The incomming and outgoing data are stored in the transaction structure
* buffers. When there is incomming data the sdp_process function must be
* called to get the data and handle the continuation state.
*
@@ -3771,7 +3771,7 @@ end:
/*
* This function starts an asynchronous service attribute request.
- * The incomming and outgoing data are stored in the transaction structure
+ * The incomming and outgoing data are stored in the transaction structure
* buffers. When there is incomming data the sdp_process function must be
* called to get the data and handle the continuation state.
*
@@ -3796,7 +3796,7 @@ end:
*
* sdp_list_t *attrid_list
* Singly linked list containing attribute identifiers desired.
- * Every element is either a uint16_t(attrSpec = SDP_ATTR_REQ_INDIVIDUAL)
+ * Every element is either a uint16_t(attrSpec = SDP_ATTR_REQ_INDIVIDUAL)
* or a uint32_t(attrSpec=SDP_ATTR_REQ_RANGE)
*
* OUTPUT :
@@ -3912,7 +3912,7 @@ end:
*
* sdp_list_t *attrid_list
* Singly linked list containing attribute identifiers desired.
- * Every element is either a uint16_t(attrSpec = SDP_ATTR_REQ_INDIVIDUAL)
+ * Every element is either a uint16_t(attrSpec = SDP_ATTR_REQ_INDIVIDUAL)
* or a uint32_t(attrSpec=SDP_ATTR_REQ_RANGE)
*
@@ -4130,7 +4130,7 @@ int sdp_process(sdp_session_t *session)
pdata += sizeof(uint16_t); /* point to csrc */
/* the first csrc contains the sum of partial csrc responses */
- *pcsrc += bt_get_unaligned((uint16_t *) pdata);
+ *pcsrc += bt_get_unaligned((uint16_t *) pdata);
pdata += sizeof(uint16_t); /* point to the first handle */
rsp_count = csrc * 4;
@@ -4141,8 +4141,8 @@ int sdp_process(sdp_session_t *session)
case SDP_SVC_SEARCH_ATTR_RSP:
rsp_count = ntohs(bt_get_unaligned((uint16_t *) pdata));
SDPDBG("Attrlist byte count : %d\n", rsp_count);
-
- /*
+
+ /*
* Number of bytes in the AttributeLists parameter(without
* continuation state) + AttributeListsByteCount field size.
*/
@@ -4168,7 +4168,7 @@ int sdp_process(sdp_session_t *session)
SDPDBG("Cstate length : %d\n", pcstate->length);
- /*
+ /*
* Check out of bound. Continuation state must have at least
* 1 byte: ZERO to indicate that it is not a partial response.
*/
@@ -4202,7 +4202,7 @@ int sdp_process(sdp_session_t *session)
// set the request header's param length
reqhdr->plen = htons(reqsize - sizeof(sdp_pdu_hdr_t));
-
+
if (sdp_send_req(session, t->reqbuf, reqsize) < 0) {
SDPERR("Error sendind data:%s(%d)", strerror(errno), errno);
status = 0xffff;
@@ -4253,7 +4253,7 @@ end:
*
* sdp_list_t *attrids
* Singly linked list containing attribute identifiers desired.
- * Every element is either a uint16_t(attrSpec = SDP_ATTR_REQ_INDIVIDUAL)
+ * Every element is either a uint16_t(attrSpec = SDP_ATTR_REQ_INDIVIDUAL)
* or a uint32_t(attrSpec=SDP_ATTR_REQ_RANGE)
*
* OUTPUT :
--
1.6.3.3
El mar, 27-04-2010 a las 18:05 +0300, Johan Hedberg escribió:
> Hi,
>
> On Tue, Apr 27, 2010, Santiago Carot Nemesio wrote:
> > This patch add memory checks after malloc function is called.
> >
> > >From 7cce3be5a1a1d506f4da71fc394ee3ccf71d8159 Mon Sep 17 00:00:00 2001
> > From: Santiago Carot Nemesio <[email protected]>
> > Date: Tue, 27 Apr 2010 15:19:44 +0200
> > Subject: [PATCH 3/3] Added memory checks
> >
> >
> > Signed-off-by: Santiago Carot Nemesio <[email protected]>
>
> I've pushed the first two patches upstream but I did have to fix their
> commit messages before that. If you try "git am" yourself for what you
> sent you'll see that the summary line becomes just "SDP" for all of them
> and it should be more descriptive than that (for examples see the bluez
> commit history).
>
> The third patch should also see these changes but additionally there
> were a few issues I noticed in the actual code:
>
> > u = malloc(sizeof(uuid_t));
> > + if (!u) {
> > + errno = ENOMEM;
> > + goto fail;
> > + }
>
I known it, but much parts in sdp.c code still set errno to ENOMEM value
when malloc fails, it is the reason which i set it to ENOMEM too.
> malloc will set the errno for you on failure so you shouldn't need to do
> it in your code.
>
> > lang = malloc(sizeof(sdp_lang_attr_t));
> > + if (!lang) {
> > + errno = ENOMEM;
> > + goto fail;
>
> Same here.
>
> > - uuid_t *uuid128 = bt_malloc(sizeof(uuid_t));
> > + uuid_t *uuid128 = malloc(sizeof(uuid_t));
>
> This seems unrelated to the other changes in the patch. I don't know the
> exact philosophy of when bt_malloc should be used and when malloc should
> be used (maybe Marcel can comment on that), but I'd simply leave this
> change out of the patch.
>
> Johan
Hi Johan,
> > This patch add memory checks after malloc function is called.
> >
> > >From 7cce3be5a1a1d506f4da71fc394ee3ccf71d8159 Mon Sep 17 00:00:00 2001
> > From: Santiago Carot Nemesio <[email protected]>
> > Date: Tue, 27 Apr 2010 15:19:44 +0200
> > Subject: [PATCH 3/3] Added memory checks
> >
> >
> > Signed-off-by: Santiago Carot Nemesio <[email protected]>
not signed-off-by in BlueZ please. That is a kernel requirement.
> I've pushed the first two patches upstream but I did have to fix their
> commit messages before that. If you try "git am" yourself for what you
> sent you'll see that the summary line becomes just "SDP" for all of them
> and it should be more descriptive than that (for examples see the bluez
> commit history).
>
> The third patch should also see these changes but additionally there
> were a few issues I noticed in the actual code:
>
> > u = malloc(sizeof(uuid_t));
> > + if (!u) {
> > + errno = ENOMEM;
> > + goto fail;
> > + }
>
> malloc will set the errno for you on failure so you shouldn't need to do
> it in your code.
>
> > lang = malloc(sizeof(sdp_lang_attr_t));
> > + if (!lang) {
> > + errno = ENOMEM;
> > + goto fail;
>
> Same here.
>
> > - uuid_t *uuid128 = bt_malloc(sizeof(uuid_t));
> > + uuid_t *uuid128 = malloc(sizeof(uuid_t));
>
> This seems unrelated to the other changes in the patch. I don't know the
> exact philosophy of when bt_malloc should be used and when malloc should
> be used (maybe Marcel can comment on that), but I'd simply leave this
> change out of the patch.
Since you are suppose to use bt_free to free the memory, you also should
allocate it with bt_malloc. They are simple wrappers, but important to
make sure all library allocation and freeing is done with the same
function. Not free() or malloc() from library A against another one form
library B.
In most cases this makes no difference, but there are corner cases where
is does make a difference. So please keep bt_malloc() if the memory
needs to be freed by the application.
Regards
Marcel
Hi,
On Tue, Apr 27, 2010, Santiago Carot Nemesio wrote:
> It may be interesting to take in mind that some functions in sdp_lib
> such as sdp_list_append or sdp_list_insert_sorted can return NULL if
> there isn't enought memory. Grepping the code i saw sentences like next:
> ...
> ap = sdp_list_append(ap, pds);
> ...
> I think that it can result in a memory leak when the asignation return
> NULL over a not empty list. What do you think?
You're right. However this seems more like a design issue with the
library than something that could be easily fixed. Either we'd have to
break the API or then go for the glib way of calling abort() on memory
allocation failures (which in a way would also be breaking the API).
Johan
Hi,
On Tue, Apr 27, 2010, Santiago Carot Nemesio wrote:
> This patch add memory checks after malloc function is called.
>
> >From 7cce3be5a1a1d506f4da71fc394ee3ccf71d8159 Mon Sep 17 00:00:00 2001
> From: Santiago Carot Nemesio <[email protected]>
> Date: Tue, 27 Apr 2010 15:19:44 +0200
> Subject: [PATCH 3/3] Added memory checks
>
>
> Signed-off-by: Santiago Carot Nemesio <[email protected]>
I've pushed the first two patches upstream but I did have to fix their
commit messages before that. If you try "git am" yourself for what you
sent you'll see that the summary line becomes just "SDP" for all of them
and it should be more descriptive than that (for examples see the bluez
commit history).
The third patch should also see these changes but additionally there
were a few issues I noticed in the actual code:
> u = malloc(sizeof(uuid_t));
> + if (!u) {
> + errno = ENOMEM;
> + goto fail;
> + }
malloc will set the errno for you on failure so you shouldn't need to do
it in your code.
> lang = malloc(sizeof(sdp_lang_attr_t));
> + if (!lang) {
> + errno = ENOMEM;
> + goto fail;
Same here.
> - uuid_t *uuid128 = bt_malloc(sizeof(uuid_t));
> + uuid_t *uuid128 = malloc(sizeof(uuid_t));
This seems unrelated to the other changes in the patch. I don't know the
exact philosophy of when bt_malloc should be used and when malloc should
be used (maybe Marcel can comment on that), but I'd simply leave this
change out of the patch.
Johan
Hi,
It may be interesting to take in mind that some functions in sdp_lib
such as sdp_list_append or sdp_list_insert_sorted can return NULL if
there isn't enought memory. Grepping the code i saw sentences like next:
...
ap = sdp_list_append(ap, pds);
...
I think that it can result in a memory leak when the asignation return
NULL over a not empty list. What do you think?
Regards.
This patch add memory checks after malloc function is called.
>>From 7cce3be5a1a1d506f4da71fc394ee3ccf71d8159 Mon Sep 17 00:00:00 2001
From: Santiago Carot Nemesio <[email protected]>
Date: Tue, 27 Apr 2010 15:19:44 +0200
Subject: [PATCH 3/3] Added memory checks
Signed-off-by: Santiago Carot Nemesio <[email protected]>
---
lib/sdp.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
1 files changed, 80 insertions(+), 7 deletions(-)
diff --git a/lib/sdp.c b/lib/sdp.c
index 667d412..64f2e96 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -1078,6 +1078,8 @@ static sdp_data_t *extract_int(const void *p, int bufsize, int *len)
}
d = malloc(sizeof(sdp_data_t));
+ if (!d)
+ return NULL;
SDPDBG("Extracting integer\n");
memset(d, 0, sizeof(sdp_data_t));
@@ -1152,6 +1154,9 @@ static sdp_data_t *extract_uuid(const uint8_t *p, int bufsize, int *len,
{
sdp_data_t *d = malloc(sizeof(sdp_data_t));
+ if (!d)
+ return NULL;
+
SDPDBG("Extracting UUID");
memset(d, 0, sizeof(sdp_data_t));
if (sdp_uuid_extract(p, bufsize, &d->val.uuid, len) < 0) {
@@ -1179,6 +1184,8 @@ static sdp_data_t *extract_str(const void *p, int bufsize, int *len)
}
d = malloc(sizeof(sdp_data_t));
+ if (!d)
+ return NULL;
memset(d, 0, sizeof(sdp_data_t));
d->dtd = *(uint8_t *) p;
@@ -1302,6 +1309,9 @@ static sdp_data_t *extract_seq(const void *p, int bufsize, int *len,
sdp_data_t *curr, *prev;
sdp_data_t *d = malloc(sizeof(sdp_data_t));
+ if (!d)
+ return NULL;
+
SDPDBG("Extracting SEQ");
memset(d, 0, sizeof(sdp_data_t));
*len = sdp_extract_seqtype(p, bufsize, &d->dtd, &seqlen);
@@ -1945,10 +1955,17 @@ int sdp_get_uuidseq_attr(const sdp_record_t *rec, uint16_t attr,
sdp_data_t *d;
for (d = sdpdata->val.dataseq; d; d = d->next) {
uuid_t *u;
- if (d->dtd < SDP_UUID16 || d->dtd > SDP_UUID128)
+ if (d->dtd < SDP_UUID16 || d->dtd > SDP_UUID128) {
+ errno = EINVAL;
goto fail;
+ }
u = malloc(sizeof(uuid_t));
+ if (!u) {
+ errno = ENOMEM;
+ goto fail;
+ }
+
memset(u, 0, sizeof(uuid_t));
*u = d->val.uuid;
*seqp = sdp_list_append(*seqp, u);
@@ -1957,7 +1974,7 @@ int sdp_get_uuidseq_attr(const sdp_record_t *rec, uint16_t attr,
}
fail:
sdp_list_free(*seqp, free);
- errno = EINVAL;
+ *seqp = NULL;
return -1;
}
@@ -1974,7 +1991,15 @@ int sdp_set_uuidseq_attr(sdp_record_t *rec, uint16_t aid, sdp_list_t *seq)
if (!seq || len == 0)
return -1;
dtds = (void **)malloc(len * sizeof(void *));
+ if (!dtds)
+ return -1;
+
values = (void **)malloc(len * sizeof(void *));
+ if (!values) {
+ free(dtds);
+ return -1;
+ }
+
for (p = seq, i = 0; i < len; i++, p = p->next) {
uuid_t *uuid = (uuid_t *)p->data;
if (uuid)
@@ -2028,6 +2053,10 @@ int sdp_get_lang_attr(const sdp_record_t *rec, sdp_list_t **langSeq)
sdp_data_t *pOffset = pEncoding->next;
if (pEncoding && pOffset) {
lang = malloc(sizeof(sdp_lang_attr_t));
+ if (!lang) {
+ errno = ENOMEM;
+ goto fail;
+ }
lang->code_ISO639 = pCode->val.uint16;
lang->encoding = pEncoding->val.uint16;
lang->base_offset = pOffset->val.uint16;
@@ -2039,6 +2068,10 @@ int sdp_get_lang_attr(const sdp_record_t *rec, sdp_list_t **langSeq)
curr_data = pOffset->next;
}
return 0;
+fail:
+ sdp_list_free(*langSeq, free);
+ *langSeq = NULL;
+ return -1;
}
int sdp_get_profile_descs(const sdp_record_t *rec, sdp_list_t **profDescSeq)
@@ -2069,6 +2102,8 @@ int sdp_get_profile_descs(const sdp_record_t *rec, sdp_list_t **profDescSeq)
if (uuid != NULL) {
profDesc = malloc(sizeof(sdp_profile_desc_t));
+ if (!profDesc)
+ goto fail;
profDesc->uuid = *uuid;
profDesc->version = version;
#ifdef SDP_DEBUG
@@ -2079,6 +2114,10 @@ int sdp_get_profile_descs(const sdp_record_t *rec, sdp_list_t **profDescSeq)
}
}
return 0;
+fail:
+ sdp_list_free(*profDescSeq, free);
+ *profDescSeq = NULL;
+ return -1;
}
int sdp_get_server_ver(const sdp_record_t *rec, sdp_list_t **u16)
@@ -2231,7 +2270,15 @@ static sdp_data_t *access_proto_to_dataseq(sdp_record_t *rec, sdp_list_t *proto)
seqlen = sdp_list_len(proto);
seqDTDs = (void **)malloc(seqlen * sizeof(void *));
+ if (!seqDTDs)
+ return NULL;
+
seqs = (void **)malloc(seqlen * sizeof(void *));
+ if (!seqs) {
+ free(seqDTDs);
+ return NULL;
+ }
+
for (i = 0, p = proto; p; p = p->next, i++) {
sdp_list_t *elt = (sdp_list_t *)p->data;
sdp_data_t *s;
@@ -2350,10 +2397,19 @@ int sdp_set_lang_attr(sdp_record_t *rec, const sdp_list_t *seq)
{
uint8_t uint16 = SDP_UINT16;
int status = 0, i = 0, seqlen = sdp_list_len(seq);
- void **dtds = (void **)malloc(3 * seqlen * sizeof(void *));
- void **values = (void **)malloc(3 * seqlen * sizeof(void *));
+ void **dtds, **values;
const sdp_list_t *p;
+ dtds = (void **)malloc(3 * seqlen * sizeof(void *));
+ if (!dtds)
+ return -1;
+
+ values = (void **)malloc(3 * seqlen * sizeof(void *));
+ if (!values) {
+ free(dtds);
+ return -1;
+ }
+
for (p = seq; p; p = p->next) {
sdp_lang_attr_t *lang = (sdp_lang_attr_t *)p->data;
if (!lang) {
@@ -2455,10 +2511,19 @@ int sdp_set_profile_descs(sdp_record_t *rec, const sdp_list_t *profiles)
uint8_t uuid128 = SDP_UUID128;
uint8_t uint16 = SDP_UINT16;
int i = 0, seqlen = sdp_list_len(profiles);
- void **seqDTDs = (void **)malloc(seqlen * sizeof(void *));
- void **seqs = (void **)malloc(seqlen * sizeof(void *));
+ void **seqDTDs, **seqs;
const sdp_list_t *p;
+ seqDTDs = (void **)malloc(seqlen * sizeof(void *));
+ if (!seqDTDs)
+ return -1;
+
+ seqs = (void **)malloc(seqlen * sizeof(void *));
+ if (!seqs) {
+ free(seqDTDs);
+ return -1;
+ }
+
for (p = profiles; p; p = p->next) {
sdp_data_t *seq;
void *dtds[2], *values[2];
@@ -2642,7 +2707,11 @@ void sdp_uuid32_to_uuid128(uuid_t *uuid128, uuid_t *uuid32)
uuid_t *sdp_uuid_to_uuid128(uuid_t *uuid)
{
- uuid_t *uuid128 = bt_malloc(sizeof(uuid_t));
+ uuid_t *uuid128 = malloc(sizeof(uuid_t));
+
+ if (!uuid128)
+ return NULL;
+
memset(uuid128, 0, sizeof(uuid_t));
switch (uuid->type) {
case SDP_UUID128:
@@ -3087,6 +3156,10 @@ int sdp_record_update(sdp_session_t *session, const sdp_record_t *rec)
sdp_record_t *sdp_record_alloc()
{
sdp_record_t *rec = malloc(sizeof(sdp_record_t));
+
+ if (!rec)
+ return NULL;
+
memset((void *)rec, 0, sizeof(sdp_record_t));
rec->handle = 0xffffffff;
return rec;
--
1.6.3.3
This patch set to NULL return value on functions returning 0.
>>From 60e2094bfdb8fcc791b8ef8a64be3e9c0c2274cc Mon Sep 17 00:00:00 2001
From: Santiago Carot Nemesio <[email protected]>
Date: Tue, 27 Apr 2010 14:09:43 +0200
Subject: [PATCH 2/3] Set to NULL return value on functions returning 0
Signed-off-by: Santiago Carot Nemesio <[email protected]>
---
lib/sdp.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/sdp.c b/lib/sdp.c
index ca16118..667d412 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -1214,7 +1214,7 @@ static sdp_data_t *extract_str(const void *p, int bufsize, int *len)
default:
SDPERR("Sizeof text string > UINT16_MAX\n");
free(d);
- return 0;
+ return NULL;
}
if (bufsize < n) {
@@ -1771,7 +1771,7 @@ sdp_list_t *sdp_list_append(sdp_list_t *p, void *d)
sdp_list_t *q, *n = malloc(sizeof(sdp_list_t));
if (!n)
- return 0;
+ return NULL;
n->data = d;
n->next = 0;
@@ -1809,7 +1809,7 @@ sdp_list_t *sdp_list_insert_sorted(sdp_list_t *list, void *d,
n = malloc(sizeof(sdp_list_t));
if (!n)
- return 0;
+ return NULL;
n->data = d;
for (q = 0, p = list; p; q = p, p = p->next)
if (f(p->data, d) >= 0)
@@ -3466,7 +3466,7 @@ sdp_record_t *sdp_service_attr_req(sdp_session_t *session, uint32_t handle,
if (reqtype != SDP_ATTR_REQ_INDIVIDUAL && reqtype != SDP_ATTR_REQ_RANGE) {
errno = EINVAL;
- return 0;
+ return NULL;
}
memset(&rsp_concat_buf, 0, sizeof(sdp_buf_t));
--
1.6.3.3