[dpdk-dev] [PATCH] cmdline: fix unchecked return value
Hi Daniel, >>> --- 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(>right, rdl->kill_buf, >> RDLINE_BUF_SIZE); >>> rdl->kill_size = CIRBUF_GET_LEN(>right); >>> - cirbuf_del_buf_head(>right, rdl->kill_size); >>> + >>> + if (cirbuf_del_buf_head(>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 ? We can mark the warning as false positive because this cannot happen right now (the calller checks the validity of cbuf/c). But this is probably something I'll come back on with a patch since there is no reason to check that pointers are not NULL in cirbuf_get_buf_head/tail(). Regards, Olivier
[dpdk-dev] [PATCH] cmdline: fix unchecked return value
>From: Olivier Matz [mailto:olivier.matz at 6wind.com] >Sent: Monday, May 02, 2016 3:37 PM >To: Mrzyglod, DanielX T ; dev at dpdk.org >Subject: Re: [PATCH] cmdline: fix unchecked return value > >Hi Daniel, > >On 04/14/2016 03:01 PM, Daniel Mrzyglod wrote: >> 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 >> --- >> 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 >> 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(>right, rdl->kill_buf, >RDLINE_BUF_SIZE); >> rdl->kill_size = CIRBUF_GET_LEN(>right); >> -cirbuf_del_buf_head(>right, rdl->kill_size); >> + >> +if (cirbuf_del_buf_head(>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
[dpdk-dev] [PATCH] cmdline: fix unchecked return value
Hi Daniel, On 04/14/2016 03:01 PM, Daniel Mrzyglod wrote: > 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 > --- > 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 > 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(>right, rdl->kill_buf, > RDLINE_BUF_SIZE); > rdl->kill_size = CIRBUF_GET_LEN(>right); > - cirbuf_del_buf_head(>right, rdl->kill_size); > + > + if (cirbuf_del_buf_head(>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
[dpdk-dev] [PATCH] cmdline: fix unchecked return value
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 --- 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 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(>right, rdl->kill_buf, RDLINE_BUF_SIZE); rdl->kill_size = CIRBUF_GET_LEN(>right); - cirbuf_del_buf_head(>right, rdl->kill_size); + + if (cirbuf_del_buf_head(>right, rdl->kill_size) < 0) + return -EINVAL; + rdline_puts(rdl, vt100_clear_right); break; @@ -496,7 +499,10 @@ rdline_char_in(struct rdline *rdl, char c) vt100_init(>vt100); cirbuf_init(>left, rdl->left_buf, 0, RDLINE_BUF_SIZE); cirbuf_init(>right, rdl->right_buf, 0, RDLINE_BUF_SIZE); - cirbuf_add_buf_tail(>left, buf, strnlen(buf, RDLINE_BUF_SIZE)); + + if (cirbuf_add_buf_tail(>left, buf, strnlen(buf, RDLINE_BUF_SIZE)) < 0) + return -EINVAL; + rdline_redisplay(rdl); break; @@ -513,7 +519,10 @@ rdline_char_in(struct rdline *rdl, char c) vt100_init(>vt100); cirbuf_init(>left, rdl->left_buf, 0, RDLINE_BUF_SIZE); cirbuf_init(>right, rdl->right_buf, 0, RDLINE_BUF_SIZE); - cirbuf_add_buf_tail(>left, buf, strnlen(buf, RDLINE_BUF_SIZE)); + + if (cirbuf_add_buf_tail(>left, buf, strnlen(buf, RDLINE_BUF_SIZE)) < 0) + return -EINVAL; + rdline_redisplay(rdl); break; @@ -640,7 +649,9 @@ rdline_add_history(struct rdline * rdl, const char * buf) rdline_remove_old_history_item(rdl); } - cirbuf_add_buf_tail(>history, buf, len); + if (cirbuf_add_buf_tail(>history, buf, len) < 0) + return -EINVAL; + cirbuf_add_tail(>history, 0); return 0; -- 2.5.5
