You are viewing a plain text version of this content. The canonical link for it is here.
Posted to proton@qpid.apache.org by Michael Ivanov <iv...@logit-ag.de> on 2015/05/26 22:25:04 UTC
pn_data_grow() issue
Greetings,
I have observed that pn_data_grow() function looses half of the available data capacity.
The following happens: when data overflows, pn_data_grow is invoked. It increases
data capacity 2 times and reallocates nodes array. Data capacity is represented as
uint16_t type and so when capacity reaches 32768 items, the result of multiplication by 2
becomes 0. This makes realloc return null and crashes the program.
To alleviate the problem with large messages I changed the function as follows:
--- qpid-proton-0.9/proton-c/src/codec/codec.c 2015-03-31 12:07:22.000000000 +0300
+++ qpid-proton-0.9.fix/proton-c/src/codec/codec.c 2015-05-26 21:18:55.801632941 +0300
@@ -417,8 +417,21 @@ void pn_data_clear(pn_data_t *data)
int pn_data_grow(pn_data_t *data)
{
- data->capacity = 2*(data->capacity ? data->capacity : 2);
- data->nodes = (pni_node_t *) realloc(data->nodes, data->capacity * sizeof(pni_node_t));
+ size_t s = data->capacity;
+
+ if (s < 0x7fff)
+ s = 2 * (s? s : 2);
+ else if (s < 0xffff - 1024)
+ s += 1024;
+ else if (s != 0xffff)
+ s = 0xffff;
+ else {
+ pn_logf("Data node %p overflow", data);
+ abort();
+ }
+
+ data->nodes = (pni_node_t *) realloc(data->nodes, s * sizeof(pni_node_t));
+ data->capacity = s;
return 0;
This allows to use capacities in 0x8000 ... 0xffff range and is supposed to report
data overflow.
Best regards,
--
\ / | |
(OvO) | Mikhail Iwanow |
(^^^) | |
\^/ | E-mail: ivans@logit-ag.de |
^ ^ | |
Re: pn_data_grow() issue
Posted by Robbie Gemmell <ro...@gmail.com>.
The JIRA project is at https://issues.apache.org/jira/browse/PROTON
Details of JIRA projects etc for each component are listed on the
website, e.g. http://qpid.apache.org/proton/ in this case.
Robbie
On 28 May 2015 at 20:46, Michael Ivanov <iv...@isle.spb.ru> wrote:
> Sorry, how do I create a JIRA issue?
>
> 27.05.2015 16:48, Alan Conway пишет:
>> On Tue, 2015-05-26 at 23:25 +0300, Michael Ivanov wrote:
>>> Greetings,
>>>
>>> I have observed that pn_data_grow() function looses half of the available data capacity.
>>> The following happens: when data overflows, pn_data_grow is invoked. It increases
>>> data capacity 2 times and reallocates nodes array. Data capacity is represented as
>>> uint16_t type and so when capacity reaches 32768 items, the result of multiplication by 2
>>> becomes 0. This makes realloc return null and crashes the program.
>>>
>>> To alleviate the problem with large messages I changed the function as follows:
>>>
>>> --- qpid-proton-0.9/proton-c/src/codec/codec.c 2015-03-31 12:07:22.000000000 +0300
>>> +++ qpid-proton-0.9.fix/proton-c/src/codec/codec.c 2015-05-26 21:18:55.801632941 +0300
>>> @@ -417,8 +417,21 @@ void pn_data_clear(pn_data_t *data)
>>>
>>> int pn_data_grow(pn_data_t *data)
>>> {
>>> - data->capacity = 2*(data->capacity ? data->capacity : 2);
>>> - data->nodes = (pni_node_t *) realloc(data->nodes, data->capacity * sizeof(pni_node_t));
>>> + size_t s = data->capacity;
>>> +
>>> + if (s < 0x7fff)
>>> + s = 2 * (s? s : 2);
>>> + else if (s < 0xffff - 1024)
>>> + s += 1024;
>>> + else if (s != 0xffff)
>>> + s = 0xffff;
>>> + else {
>>> + pn_logf("Data node %p overflow", data);
>>> + abort();
>>> + }
>>> +
>>> + data->nodes = (pni_node_t *) realloc(data->nodes, s * sizeof(pni_node_t));
>>> + data->capacity = s;
>>> return 0;
>>>
>>> This allows to use capacities in 0x8000 ... 0xffff range and is supposed to report
>>> data overflow.
>>
>>
>> Good catch, can you please create a JIRA with the patch attached so you
>> can get proper credit for it (for copyright reasons I can't take the
>> patch direct from email.) You can assign it to me, I'll apply it right
>> away.
>>
>> I would suggest a couple of changes:
>>
>> 1. Define an explicit PNI_NID_MAX in data.h right beside typedef
>> pni_nid_t rather than using literal 0xffff etc. in case someone changes
>> the definition of pni_nid_t at some point.
>>
>> 2. Use a simple double or max rather than adding 1k after 0x7fff. We
>> need to fix the algorithm but I don't think we need to add new
>> complexity unless there's a separate justification for that.
>>
>> 3. If we fail to increase (i.e. we're already at PNI_NID_MAX) return
>> PN_OVERFLOW rather than aborting. There is only one function
>> (pn_data_new) that calls pn_data_grow, make it check the return value
>> and return NULL on error. That won't make exiting code worse (it will
>> still crash on overrun) but will give well written code that checks
>> error values the choice.
>>
>> Thanks for this!
>> Alan.
>>
>
>
> --
> \ / | |
> (OvO) | Михаил Иванов |
> (^^^) | Тел.: +7(911) 223-1300 |
> \^/ | E-mail: ivans@isle.spb.ru |
> ^ ^ | |
Re: pn_data_grow() issue
Posted by Michael Ivanov <iv...@isle.spb.ru>.
Sorry, how do I create a JIRA issue?
27.05.2015 16:48, Alan Conway пишет:
> On Tue, 2015-05-26 at 23:25 +0300, Michael Ivanov wrote:
>> Greetings,
>>
>> I have observed that pn_data_grow() function looses half of the available data capacity.
>> The following happens: when data overflows, pn_data_grow is invoked. It increases
>> data capacity 2 times and reallocates nodes array. Data capacity is represented as
>> uint16_t type and so when capacity reaches 32768 items, the result of multiplication by 2
>> becomes 0. This makes realloc return null and crashes the program.
>>
>> To alleviate the problem with large messages I changed the function as follows:
>>
>> --- qpid-proton-0.9/proton-c/src/codec/codec.c 2015-03-31 12:07:22.000000000 +0300
>> +++ qpid-proton-0.9.fix/proton-c/src/codec/codec.c 2015-05-26 21:18:55.801632941 +0300
>> @@ -417,8 +417,21 @@ void pn_data_clear(pn_data_t *data)
>>
>> int pn_data_grow(pn_data_t *data)
>> {
>> - data->capacity = 2*(data->capacity ? data->capacity : 2);
>> - data->nodes = (pni_node_t *) realloc(data->nodes, data->capacity * sizeof(pni_node_t));
>> + size_t s = data->capacity;
>> +
>> + if (s < 0x7fff)
>> + s = 2 * (s? s : 2);
>> + else if (s < 0xffff - 1024)
>> + s += 1024;
>> + else if (s != 0xffff)
>> + s = 0xffff;
>> + else {
>> + pn_logf("Data node %p overflow", data);
>> + abort();
>> + }
>> +
>> + data->nodes = (pni_node_t *) realloc(data->nodes, s * sizeof(pni_node_t));
>> + data->capacity = s;
>> return 0;
>>
>> This allows to use capacities in 0x8000 ... 0xffff range and is supposed to report
>> data overflow.
>
>
> Good catch, can you please create a JIRA with the patch attached so you
> can get proper credit for it (for copyright reasons I can't take the
> patch direct from email.) You can assign it to me, I'll apply it right
> away.
>
> I would suggest a couple of changes:
>
> 1. Define an explicit PNI_NID_MAX in data.h right beside typedef
> pni_nid_t rather than using literal 0xffff etc. in case someone changes
> the definition of pni_nid_t at some point.
>
> 2. Use a simple double or max rather than adding 1k after 0x7fff. We
> need to fix the algorithm but I don't think we need to add new
> complexity unless there's a separate justification for that.
>
> 3. If we fail to increase (i.e. we're already at PNI_NID_MAX) return
> PN_OVERFLOW rather than aborting. There is only one function
> (pn_data_new) that calls pn_data_grow, make it check the return value
> and return NULL on error. That won't make exiting code worse (it will
> still crash on overrun) but will give well written code that checks
> error values the choice.
>
> Thanks for this!
> Alan.
>
--
\ / | |
(OvO) | Михаил Иванов |
(^^^) | Тел.: +7(911) 223-1300 |
\^/ | E-mail: ivans@isle.spb.ru |
^ ^ | |
Re: pn_data_grow() issue
Posted by Alan Conway <ac...@redhat.com>.
On Tue, 2015-05-26 at 23:25 +0300, Michael Ivanov wrote:
> Greetings,
>
> I have observed that pn_data_grow() function looses half of the available data capacity.
> The following happens: when data overflows, pn_data_grow is invoked. It increases
> data capacity 2 times and reallocates nodes array. Data capacity is represented as
> uint16_t type and so when capacity reaches 32768 items, the result of multiplication by 2
> becomes 0. This makes realloc return null and crashes the program.
>
> To alleviate the problem with large messages I changed the function as follows:
>
> --- qpid-proton-0.9/proton-c/src/codec/codec.c 2015-03-31 12:07:22.000000000 +0300
> +++ qpid-proton-0.9.fix/proton-c/src/codec/codec.c 2015-05-26 21:18:55.801632941 +0300
> @@ -417,8 +417,21 @@ void pn_data_clear(pn_data_t *data)
>
> int pn_data_grow(pn_data_t *data)
> {
> - data->capacity = 2*(data->capacity ? data->capacity : 2);
> - data->nodes = (pni_node_t *) realloc(data->nodes, data->capacity * sizeof(pni_node_t));
> + size_t s = data->capacity;
> +
> + if (s < 0x7fff)
> + s = 2 * (s? s : 2);
> + else if (s < 0xffff - 1024)
> + s += 1024;
> + else if (s != 0xffff)
> + s = 0xffff;
> + else {
> + pn_logf("Data node %p overflow", data);
> + abort();
> + }
> +
> + data->nodes = (pni_node_t *) realloc(data->nodes, s * sizeof(pni_node_t));
> + data->capacity = s;
> return 0;
>
> This allows to use capacities in 0x8000 ... 0xffff range and is supposed to report
> data overflow.
Good catch, can you please create a JIRA with the patch attached so you
can get proper credit for it (for copyright reasons I can't take the
patch direct from email.) You can assign it to me, I'll apply it right
away.
I would suggest a couple of changes:
1. Define an explicit PNI_NID_MAX in data.h right beside typedef
pni_nid_t rather than using literal 0xffff etc. in case someone changes
the definition of pni_nid_t at some point.
2. Use a simple double or max rather than adding 1k after 0x7fff. We
need to fix the algorithm but I don't think we need to add new
complexity unless there's a separate justification for that.
3. If we fail to increase (i.e. we're already at PNI_NID_MAX) return
PN_OVERFLOW rather than aborting. There is only one function
(pn_data_new) that calls pn_data_grow, make it check the return value
and return NULL on error. That won't make exiting code worse (it will
still crash on overrun) but will give well written code that checks
error values the choice.
Thanks for this!
Alan.