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.