diff mbox series

scsi: target: iscsi: Use strcmp() instead of strncmp()

Message ID 20220127084754.GA25644@kili (mailing list archive)
State New, archived
Headers show
Series scsi: target: iscsi: Use strcmp() instead of strncmp() | expand

Commit Message

Dan Carpenter Jan. 27, 2022, 8:47 a.m. UTC
We want to match the whole string "=All" and this code does that, but
strncmp() is normally used for a partial match and it's more readable
to use strcmp().

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/target/iscsi/iscsi_target.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Mark Mielke Jan. 30, 2022, 6 p.m. UTC | #1
Sorry, I knee jerk react to "partial match", which was a red flag for
me, so strncmp() is almost never used for "partial match" in my
experience. :-)

It's really that strncmp() should be used for buffer overflow
protection, but in this case - it's not. Since, the ", 5" applies to
the clearly visible "=All" that is both allocated and '\0' terminated.
It should be against the remaining buffer size of text_ptr. So,
removing the ", 5" leaves it equally as bad at buffer overflow
protection as it was before. :-)

On Thu, Jan 27, 2022 at 10:19 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> We want to match the whole string "=All" and this code does that, but
> strncmp() is normally used for a partial match and it's more readable
> to use strcmp().
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/target/iscsi/iscsi_target.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 2c54c5d8412d..4534101a7376 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -2213,7 +2213,7 @@ iscsit_process_text_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
>         /* '=' confirmed in strncmp */
>         text_ptr = strchr(text_in, '=');
>         BUG_ON(!text_ptr);
> -       if (!strncmp("=All", text_ptr, 5)) {
> +       if (!strcmp("=All", text_ptr)) {
>                 cmd->cmd_flags |= ICF_SENDTARGETS_ALL;
>         } else if (!strncmp("=iqn.", text_ptr, 5) ||
>                    !strncmp("=eui.", text_ptr, 5)) {
> --
> 2.20.1
>
Dan Carpenter Feb. 1, 2022, 11:58 a.m. UTC | #2
On Sun, Jan 30, 2022 at 01:00:21PM -0500, Mark Mielke wrote:
> Sorry, I knee jerk react to "partial match", which was a red flag for
> me, so strncmp() is almost never used for "partial match" in my
> experience. :-)
> 
> It's really that strncmp() should be used for buffer overflow
> protection, but in this case - it's not. Since, the ", 5" applies to
> the clearly visible "=All" that is both allocated and '\0' terminated.
> It should be against the remaining buffer size of text_ptr. So,
> removing the ", 5" leaves it equally as bad at buffer overflow
> protection as it was before. :-)

This isn't really about my patch we all agree leaves things "as it
was before".  In this case, strncmp() was just copy and paste from the
surrounding code and not used as an overflow check.

What you're describing does exist but it's about 1% of use cases.  There
are about 2868 calls to strncmp() in the kernel.  In my x86 allmodconfig
only about 18 of them use strncmp() for buffer overflow checking.

Then there are some real puzzlers which do:

	if (strncmp(variable, "foo", sizeof("foo")) == 0) {

regards,
dan carpenter
diff mbox series

Patch

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 2c54c5d8412d..4534101a7376 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -2213,7 +2213,7 @@  iscsit_process_text_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 	/* '=' confirmed in strncmp */
 	text_ptr = strchr(text_in, '=');
 	BUG_ON(!text_ptr);
-	if (!strncmp("=All", text_ptr, 5)) {
+	if (!strcmp("=All", text_ptr)) {
 		cmd->cmd_flags |= ICF_SENDTARGETS_ALL;
 	} else if (!strncmp("=iqn.", text_ptr, 5) ||
 		   !strncmp("=eui.", text_ptr, 5)) {