| Message ID | 1419334799-19653-4-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 23 December 2014 at 12:39, Maxim Uvarov <maxim.uvarov@linaro.org> 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 <maxim.uvarov@linaro.org> > --- > 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 > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
_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?
On Tue, Dec 23, 2014 at 7:10 AM, Ola Liljedahl <ola.liljedahl@linaro.org>
wrote:
> On 23 December 2014 at 12:39, Maxim Uvarov <maxim.uvarov@linaro.org>
> 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 <maxim.uvarov@linaro.org>
> > ---
> > 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
> > lng-odp@lists.linaro.org
> > http://lists.linaro.org/mailman/listinfo/lng-odp
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
On 23 December 2014 at 14:28, Bill Fischofer <bill.fischofer@linaro.org> 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 <ola.liljedahl@linaro.org> > wrote: >> >> On 23 December 2014 at 12:39, Maxim Uvarov <maxim.uvarov@linaro.org> >> 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 <maxim.uvarov@linaro.org> >> > --- >> > 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 >> > lng-odp@lists.linaro.org >> > http://lists.linaro.org/mailman/listinfo/lng-odp >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp > >
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 <ola.liljedahl@linaro.org> wrote: > > On 23 December 2014 at 14:28, Bill Fischofer <bill.fischofer@linaro.org> > 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 <ola.liljedahl@linaro.org > > > > wrote: > >> > >> On 23 December 2014 at 12:39, Maxim Uvarov <maxim.uvarov@linaro.org> > >> 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 <maxim.uvarov@linaro.org> > >> > --- > >> > 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 > >> > lng-odp@lists.linaro.org > >> > http://lists.linaro.org/mailman/listinfo/lng-odp > >> > >> _______________________________________________ > >> lng-odp mailing list > >> lng-odp@lists.linaro.org > >> http://lists.linaro.org/mailman/listinfo/lng-odp > > > > >
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 > <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>> wrote: > > On 23 December 2014 at 12:39, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> 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 <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > > --- > > 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 > > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > > http://lists.linaro.org/mailman/listinfo/lng-odp > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp > >
OK, expect that patch shortly. Bill On Tue, Dec 23, 2014 at 8:56 AM, Maxim Uvarov <maxim.uvarov@linaro.org> 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 <ola.liljedahl@linaro.org >> <mailto:ola.liljedahl@linaro.org>> wrote: >> >> On 23 December 2014 at 12:39, Maxim Uvarov >> <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> 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 <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> >> >> > --- >> > 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 >> > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> > http://lists.linaro.org/mailman/listinfo/lng-odp >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >
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)); + if (0 != ret) + abort(); + if (completion_event == odp_packet_to_buffer(params->pkt)) completion_event = odp_packet_to_buffer(params->out_pkt);
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 <maxim.uvarov@linaro.org> --- platform/linux-generic/odp_crypto.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)