[PATCH] scsi: target: iscsi: Use strcmp() instead of strncmp()

[PATCH] scsi: target: iscsi: Use strcmp() instead of strncmp()

[Dan Carpenter]

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

Re: [PATCH] scsi: target: iscsi: Use strcmp() instead of strncmp()

[Mark Mielke]

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
>


-- 
Mark Mielke <mark.mielke@gmail.com>

Re: [PATCH] scsi: target: iscsi: Use strcmp() instead of strncmp()

[Dan Carpenter]

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