The RC is there to indicate that the requested copy range is not within the bounds of either the source or destination packet. In this case that would not be the case and so this call could never return a non-zero value, but in the general case you still need the routine to make this check to avoid spurious buffer overruns.
On Tue, Dec 23, 2014 at 8:28 AM, Ola Liljedahl <[email protected]> wrote: > > 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
