Re: pn_data_grow() issue
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.0 +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_ts = data-capacity;
+
+ if (s 0x7fff)
+ s = 2 * (s? s : 2);
+ else if (s 0x - 1024)
+ s += 1024;
+ else if (s != 0x)
+ s = 0x;
+ 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 ... 0x 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 0x 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: iv...@isle.spb.ru |
^ ^ | |
Re: pn_data_grow() issue
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.0 +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_ts = data-capacity;
+
+ if (s 0x7fff)
+ s = 2 * (s? s : 2);
+ else if (s 0x - 1024)
+ s += 1024;
+ else if (s != 0x)
+ s = 0x;
+ 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 ... 0x 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 0x 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: iv...@isle.spb.ru |
^ ^ | |
Re: pn_data_grow() issue
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.c2015-03-31
12:07:22.0 +0300
+++ qpid-proton-0.9.fix/proton-c/src/codec/codec.c2015-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_ts = data-capacity;
+
+ if (s 0x7fff)
+ s = 2 * (s? s : 2);
+ else if (s 0x - 1024)
+ s += 1024;
+ else if (s != 0x)
+ s = 0x;
+ 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 ... 0x 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 0x 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.
