OK, expect that patch shortly. Bill
On Tue, Dec 23, 2014 at 8:56 AM, Maxim Uvarov <[email protected]> wrote: > > > On 12/23/2014 04:28 PM, Bill Fischofer wrote: > >> _odo_packet_copy_to_packet() will fail if either the source or target >> offset/length is out of range. In this case it can only happen if out_pkt >> is shorter than pkt. The problem here is that this code should be using >> odp_packet_alloc() instead of odp_buffer_alloc() (strong typechecking would >> flag this as an error) and specifying the correct length of the packet it >> needs. Here's the fuller context of the code in question: >> >> /* Resolve output buffer */ >> if (ODP_PACKET_INVALID == params->out_pkt) >> if (ODP_BUFFER_POOL_INVALID != session->output_pool) >> params->out_pkt = >> odp_buffer_alloc(session->output_pool); >> if (params->pkt != params->out_pkt) { >> if (odp_unlikely(ODP_PACKET_INVALID == params->out_pkt)) >> abort(); >> _odp_packet_copy_to_packet(params->pkt, 0, params->out_pkt, 0, >> odp_packet_len(params->pkt)); >> if (completion_event == odp_packet_to_buffer(params->pkt)) >> completion_event = >> odp_packet_to_buffer(params->out_pkt); >> odp_packet_free(params->pkt); >> params->pkt = ODP_PACKET_INVALID; >> } >> >> The correct code should be: >> >> /* Resolve output buffer */ >> if (ODP_PACKET_INVALID == params->out_pkt) >> if (ODP_BUFFER_POOL_INVALID != session->output_pool) >> params->out_pkt = >> odp_packet_alloc(session->output_pool, >> odp_packet_len(params->pkt)); >> if (params->pkt != params->out_pkt) { >> if (odp_unlikely(ODP_PACKET_INVALID == params->out_pkt)) >> abort(); >> _odp_packet_copy_to_packet(params->pkt, 0, params->out_pkt, 0, >> odp_packet_len(params->pkt)); >> if (completion_event == odp_packet_to_buffer(params->pkt)) >> completion_event = >> odp_packet_to_buffer(params->out_pkt); >> odp_packet_free(params->pkt); >> params->pkt = ODP_PACKET_INVALID; >> } >> >> With this change _odp_packet_copy_to_packet() cannot return a non-zero >> value. >> >> So this is a bug in the original patch and should be corrected as noted >> above. Maxim: did you want to change your patch or should I post this bug >> fix? >> >> > Bill I think Coverity complained that _odp_packet_copy_to_packet() is int > function and return value is not checked. Even it it's returns only 0 > Coverety might complain about it. Fill free to send your version of patch. > > Maxim. > > >> On Tue, Dec 23, 2014 at 7:10 AM, Ola Liljedahl <[email protected] >> <mailto:[email protected]>> wrote: >> >> On 23 December 2014 at 12:39, Maxim Uvarov >> <[email protected] <mailto:[email protected]>> wrote: >> > CID 85004: Unchecked return value (CHECKED_RETURN) >> > Calling "_odp_packet_copy_to_packet" without checking >> > return value (as is done elsewhere 5 out of 6 times). >> > >> > Signed-off-by: Maxim Uvarov <[email protected] >> <mailto:[email protected]>> >> >> > --- >> > platform/linux-generic/odp_crypto.c | 8 ++++++-- >> > 1 file changed, 6 insertions(+), 2 deletions(-) >> > >> > diff --git a/platform/linux-generic/odp_crypto.c >> b/platform/linux-generic/odp_crypto.c >> > index 13c5556..09adda1 100644 >> > --- a/platform/linux-generic/odp_crypto.c >> > +++ b/platform/linux-generic/odp_crypto.c >> > @@ -350,6 +350,7 @@ odp_crypto_operation(odp_crypto_op_params_t >> *params, >> > enum crypto_alg_err rc_auth = ODP_CRYPTO_ALG_ERR_NONE; >> > odp_crypto_generic_session_t *session; >> > odp_crypto_generic_op_result_t *result; >> > + int ret; >> > >> > *posted = 0; >> > session = (odp_crypto_generic_session_t >> *)(intptr_t)params->session; >> > @@ -362,8 +363,11 @@ odp_crypto_operation(odp_crypto_op_params_t >> *params, >> > if (params->pkt != params->out_pkt) { >> > if (odp_unlikely(ODP_PACKET_INVALID == >> params->out_pkt)) >> > abort(); >> > - _odp_packet_copy_to_packet(params->pkt, 0, params->out_pkt, 0, >> > - odp_packet_len(params->pkt)); >> > + ret = _odp_packet_copy_to_packet(params->pkt, 0, >> params->out_pkt, 0, >> > + odp_packet_len(params->pkt)); >> When can odp_packet_copy_to_packet() fail? Because we need to allocate >> more buffer/segments? Buffer exhaustion is a non-fatal error in a >> networking application and we cannot abort the program. Roll back this >> operation and return an error to the caller. >> >> > + if (0 != ret) >> > + abort(); >> > + >> > if (completion_event == >> odp_packet_to_buffer(params->pkt)) >> > completion_event = >> > odp_packet_to_buffer(params->out_pkt); >> > -- >> > 1.8.5.1.163.gd7aced9 >> > >> > >> > _______________________________________________ >> > lng-odp mailing list >> > [email protected] <mailto:[email protected]> >> > http://lists.linaro.org/mailman/listinfo/lng-odp >> >> _______________________________________________ >> lng-odp mailing list >> [email protected] <mailto:[email protected]> >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
