From: Olivier Matz [mailto:olivier.matz@6wind.com]
Sent: Monday, May 02, 2016 3:37 PM
To: Mrzyglod, DanielX T <redacted>; dev@dpdk.org
Subject: Re: [PATCH] cmdline: fix unchecked return value

Hi Daniel,

On 04/14/2016 03:01 PM, Daniel Mrzyglod wrote:

quoted

This patch is for checking if error values occurs.
fix for coverity errors #13209 & #13195

If the function returns an error value, the error value may be mistaken
for a normal value.

In rdline_char_in: Value returned from a function is not checked for errors
before being used

Signed-off-by: Daniel Mrzyglod <redacted>
---
 lib/librte_cmdline/cmdline_rdline.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/lib/librte_cmdline/cmdline_rdline.c

b/lib/librte_cmdline/cmdline_rdline.c

quoted

index 1ef2258..e75a556 100644

--- a/lib/librte_cmdline/cmdline_rdline.c
+++ b/lib/librte_cmdline/cmdline_rdline.c

@@ -377,7 +377,10 @@ rdline_char_in(struct rdline *rdl, char c)
 		case CMDLINE_KEY_CTRL_K:
 			cirbuf_get_buf_head(&rdl->right, rdl->kill_buf,

RDLINE_BUF_SIZE);

quoted

 			rdl->kill_size = CIRBUF_GET_LEN(&rdl->right);
-			cirbuf_del_buf_head(&rdl->right, rdl->kill_size);
+
+			if (cirbuf_del_buf_head(&rdl->right, rdl->kill_size) < 0)
+					return -EINVAL;
+
 			rdline_puts(rdl, vt100_clear_right);
 			break;

I wonder if a better way to fix wouldn't be to remove the checks
introduced in http://dpdk.org/browse/dpdk/commit/?id=ab971e562860

There is no reason to check that in cirbuf_get_buf_head/tail():
   if (!cbuf || !c)

The function should never fail, it just returns the number of
copied chars. This is the responsibility of the caller to ensure
that the pointer to the circular buffer is not NULL.

Also, rdline_char_in() is not expected to return -EINVAL, but
RDLINE_RES_* instead.

So I think that partially revert ab971e562860 would fix the
coverity warning.

Regards,
Olivier

Removing checks probably will generate more Coverity errors somewhere.
I see that only places where we test negative values are in unit tests.

Reverting changes I think is overhead and maybe ignoring this patch and set is as false positive in Coverity is better idea ?

Regards
Daniel