[PATCH] target: iscsi: iscsi_target_tpg.c: Cleaning up possible size overwriting in conjunction with sprintf

[PATCH] target: iscsi: iscsi_target_tpg.c: Cleaning up possible size overwriting in conjunction with sprintf

[Rickard Strandqvist]

Changed same snprintf and sprintf to strlcpy and strlcat.
This will guarantee that the string size is not overwritten,
and they are significantly faster than sprintf also.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/target/iscsi/iscsi_target_tpg.c |   16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_tpg.c b/drivers/target/iscsi/iscsi_target_tpg.c
index c3cb5c1..6fc8bfe 100644
--- a/drivers/target/iscsi/iscsi_target_tpg.c
+++ b/drivers/target/iscsi/iscsi_target_tpg.c
@@ -608,7 +608,6 @@ int iscsit_tpg_set_initiator_node_queue_depth(
 int iscsit_ta_authentication(struct iscsi_portal_group *tpg, u32 authentication)
 {
 	unsigned char buf1[256], buf2[256], *none = NULL;
-	int len;
 	struct iscsi_param *param;
 	struct iscsi_tpg_attrib *a = &tpg->tpg_attrib;
 
@@ -626,34 +625,33 @@ int iscsit_ta_authentication(struct iscsi_portal_group *tpg, u32 authentication)
 		return -EINVAL;
 
 	if (authentication) {
-		snprintf(buf1, sizeof(buf1), "%s", param->value);
+		strlcpy(buf1, param->value, sizeof(buf1));
 		none = strstr(buf1, NONE);
 		if (!none)
 			goto out;
 		if (!strncmp(none + 4, ",", 1)) {
 			if (!strcmp(buf1, none))
-				sprintf(buf2, "%s", none+5);
+				strlcpy(buf2, none+5, sizeof(buf2));
 			else {
 				none--;
 				*none = '\0';
-				len = sprintf(buf2, "%s", buf1);
+				strlcpy(buf2, buf1, sizeof(buf2));
 				none += 5;
-				sprintf(buf2 + len, "%s", none);
+				strlcat(buf2, none, sizeof(buf2));
 			}
 		} else {
 			none--;
 			*none = '\0';
-			sprintf(buf2, "%s", buf1);
+			strlcpy(buf2, buf1, sizeof(buf2));
 		}
 		if (iscsi_update_param_value(param, buf2) < 0)
 			return -EINVAL;
 	} else {
-		snprintf(buf1, sizeof(buf1), "%s", param->value);
+		strlcpy(buf1, param->value, sizeof(buf1));
 		none = strstr(buf1, NONE);
 		if (none)
 			goto out;
-		strncat(buf1, ",", strlen(","));
-		strncat(buf1, NONE, strlen(NONE));
+		strlcat(buf1, ","  NONE , sizeof(buf1));
 		if (iscsi_update_param_value(param, buf1) < 0)
 			return -EINVAL;
 	}
-- 
1.7.10.4

Re: [PATCH] target: iscsi: iscsi_target_tpg.c: Cleaning up possible size overwriting in conjunction with sprintf

[Joe Perches]

On Sun, 2014-10-12 at 19:55 +0200, Rickard Strandqvist wrote:
> Changed same snprintf and sprintf to strlcpy and strlcat.
> This will guarantee that the string size is not overwritten,
> and they are significantly faster than sprintf also.

I don't disagree with the change, but I think the changelog is
misleading.

strlcpy(ptr, ...) then strlcat(ptr, ...) does not use the known
length of the first copied string and is generally not faster than
ptr += snprintf(ptr, ...) then sprintf(ptr,...)

And a couple slightly unrelated bits:

> diff --git a/drivers/target/iscsi/iscsi_target_tpg.c b/drivers/target/iscsi/iscsi_target_tpg.c
> index c3cb5c1..6fc8bfe 100644
> --- a/drivers/target/iscsi/iscsi_target_tpg.c
> +++ b/drivers/target/iscsi/iscsi_target_tpg.c
> @@ -608,7 +608,6 @@ int iscsit_tpg_set_initiator_node_queue_depth(
>  int iscsit_ta_authentication(struct iscsi_portal_group *tpg, u32 authentication)

This function could be made static and the prototype removed from
the .h file if it is moved above its one caller in this file.

>  {
>  	unsigned char buf1[256], buf2[256], *none = NULL;

Moderately large stack use here.

Re: [PATCH] target: iscsi: iscsi_target_tpg.c: Cleaning up possible size overwriting in conjunction with sprintf

[Rickard Strandqvist]

2014-10-12 20:18 GMT+02:00 Joe Perches <joe@perches.com>:
> On Sun, 2014-10-12 at 19:55 +0200, Rickard Strandqvist wrote:
>> Changed same snprintf and sprintf to strlcpy and strlcat.
>> This will guarantee that the string size is not overwritten,
>> and they are significantly faster than sprintf also.
>
> I don't disagree with the change, but I think the changelog is
> misleading.
>
> strlcpy(ptr, ...) then strlcat(ptr, ...) does not use the known
> length of the first copied string and is generally not faster than
> ptr += snprintf(ptr, ...) then sprintf(ptr,...)
>
> And a couple slightly unrelated bits:
>
>> diff --git a/drivers/target/iscsi/iscsi_target_tpg.c b/drivers/target/iscsi/iscsi_target_tpg.c
>> index c3cb5c1..6fc8bfe 100644
>> --- a/drivers/target/iscsi/iscsi_target_tpg.c
>> +++ b/drivers/target/iscsi/iscsi_target_tpg.c
>> @@ -608,7 +608,6 @@ int iscsit_tpg_set_initiator_node_queue_depth(
>>  int iscsit_ta_authentication(struct iscsi_portal_group *tpg, u32 authentication)
>
> This function could be made static and the prototype removed from
> the .h file if it is moved above its one caller in this file.
>
>>  {
>>       unsigned char buf1[256], buf2[256], *none = NULL;
>
> Moderately large stack use here.
>



Hi

Can not agree with you about speed.
But regardless of that, it is ironically precisely the behavior you
describe that is a part of the problem.
Form the man page for snprintf
"If the output was truncated due to this limit then the return value
is the number of characters (excluding the terminating null byte)
which would have been written to the final string if enough space had
been available"

Switching to static funktion sounds good.
And less use of static memory, anyone have a more reasonable idea of a size?


Kind regards
Rickard Strandqvist