On 23 December 2014 at 14:28, Bill Fischofer <[email protected]> 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 _odp_packet_copy_to_packet() wouldn't be able to fail then? Is there any need for a return value then? Just to return the number of bytes copied? Sorry I haven't looked to close at this.
> > 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? > > > On Tue, Dec 23, 2014 at 7:10 AM, Ola Liljedahl <[email protected]> > wrote: >> >> On 23 December 2014 at 12:39, Maxim Uvarov <[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]> >> > --- >> > 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] >> > http://lists.linaro.org/mailman/listinfo/lng-odp >> >> _______________________________________________ >> lng-odp mailing list >> [email protected] >> http://lists.linaro.org/mailman/listinfo/lng-odp > > _______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
