[KJ] [patch] net/tipc: sprintf/strcpy conversion

[KJ] [patch] net/tipc: sprintf/strcpy conversion

[Florian Westphal]

From: Florian Westphal <fw@strlen.de>

convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.

Signed-off-by: Florian Westphal <fw@strlen.de>

---

compile tested; diffed against davem/net-2.6.

--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -119,7 +119,7 @@ static struct bclink *bclink = NULL;
 static struct link *bcl = NULL;
 static DEFINE_SPINLOCK(bc_lock);
 
-char tipc_bclink_name[] = "multicast-link";
+const char tipc_bclink_name[] = "multicast-link";
 
 
 static u32 buf_seqno(struct sk_buff *buf)
@@ -790,7 +790,7 @@ int tipc_bclink_init(void)
 	INIT_LIST_HEAD(&bcbearer->bearer.cong_links);
 	bcbearer->bearer.media = &bcbearer->media;
 	bcbearer->media.send_msg = tipc_bcbearer_send;
-	sprintf(bcbearer->media.name, "tipc-multicast");
+	strcpy(bcbearer->media.name, "tipc-multicast");
 
 	bcl = &bclink->link;
 	memset(bclink, 0, sizeof(struct bclink));
@@ -802,7 +802,7 @@ int tipc_bclink_init(void)
 	tipc_link_set_queue_limits(bcl, BCLINK_WIN_DEFAULT);
 	bcl->b_ptr = &bcbearer->bearer;
 	bcl->state = WORKING_WORKING;
-	sprintf(bcl->name, tipc_bclink_name);
+	strcpy(bcl->name, tipc_bclink_name);
 
 	if (BCLINK_LOG_BUF_SIZE) {
 		char *pb = kmalloc(BCLINK_LOG_BUF_SIZE, GFP_ATOMIC);
diff --git a/net/tipc/bcast.h b/net/tipc/bcast.h
--- a/net/tipc/bcast.h
+++ b/net/tipc/bcast.h
@@ -70,7 +70,7 @@ struct port_list {
 
 struct node;
 
-extern char tipc_bclink_name[];
+extern const char tipc_bclink_name[];
 
 
 /**
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -667,7 +667,7 @@ struct sk_buff *tipc_node_get_links(cons
         link_info.dest = tipc_own_addr & 0xfffff00;
 	link_info.dest = htonl(link_info.dest);
         link_info.up = htonl(1);
-        sprintf(link_info.str, tipc_bclink_name);
+        strcpy(link_info.str, tipc_bclink_name);
 	tipc_cfg_append_tlv(buf, TIPC_TLV_LINK_INFO, &link_info, sizeof(link_info));
 
 	/* Add TLVs for any other links in scope */
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[patch] net/tipc: sprintf/strcpy conversion

[Florian Westphal]

From: Florian Westphal <fw@strlen.de>

convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.

Signed-off-by: Florian Westphal <fw@strlen.de>

---

compile tested; diffed against davem/net-2.6.

--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -119,7 +119,7 @@ static struct bclink *bclink = NULL;
 static struct link *bcl = NULL;
 static DEFINE_SPINLOCK(bc_lock);
 
-char tipc_bclink_name[] = "multicast-link";
+const char tipc_bclink_name[] = "multicast-link";
 
 
 static u32 buf_seqno(struct sk_buff *buf)
@@ -790,7 +790,7 @@ int tipc_bclink_init(void)
 	INIT_LIST_HEAD(&bcbearer->bearer.cong_links);
 	bcbearer->bearer.media = &bcbearer->media;
 	bcbearer->media.send_msg = tipc_bcbearer_send;
-	sprintf(bcbearer->media.name, "tipc-multicast");
+	strcpy(bcbearer->media.name, "tipc-multicast");
 
 	bcl = &bclink->link;
 	memset(bclink, 0, sizeof(struct bclink));
@@ -802,7 +802,7 @@ int tipc_bclink_init(void)
 	tipc_link_set_queue_limits(bcl, BCLINK_WIN_DEFAULT);
 	bcl->b_ptr = &bcbearer->bearer;
 	bcl->state = WORKING_WORKING;
-	sprintf(bcl->name, tipc_bclink_name);
+	strcpy(bcl->name, tipc_bclink_name);
 
 	if (BCLINK_LOG_BUF_SIZE) {
 		char *pb = kmalloc(BCLINK_LOG_BUF_SIZE, GFP_ATOMIC);
diff --git a/net/tipc/bcast.h b/net/tipc/bcast.h
--- a/net/tipc/bcast.h
+++ b/net/tipc/bcast.h
@@ -70,7 +70,7 @@ struct port_list {
 
 struct node;
 
-extern char tipc_bclink_name[];
+extern const char tipc_bclink_name[];
 
 
 /**
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -667,7 +667,7 @@ struct sk_buff *tipc_node_get_links(cons
         link_info.dest = tipc_own_addr & 0xfffff00;
 	link_info.dest = htonl(link_info.dest);
         link_info.up = htonl(1);
-        sprintf(link_info.str, tipc_bclink_name);
+        strcpy(link_info.str, tipc_bclink_name);
 	tipc_cfg_append_tlv(buf, TIPC_TLV_LINK_INFO, &link_info, sizeof(link_info));
 
 	/* Add TLVs for any other links in scope */

Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion

[walter harms]

hi Florian,

These line
+	strcpy(bcbearer->media.name, "tipc-multicast");

i gues that means tipc_bclink_name ?
an even more secure version could be like this:

	strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name);

(in case someone ever changes the size of cbearer->media.name or tipc_bclink_name and the hope
that wchat_t will never reach the kernel)

re,
 wh




Florian Westphal wrote:
> From: Florian Westphal <fw@strlen.de>
> 
> convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> 
> ---
> 
> compile tested; diffed against davem/net-2.6.
> 
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -119,7 +119,7 @@ static struct bclink *bclink = NULL;
>  static struct link *bcl = NULL;
>  static DEFINE_SPINLOCK(bc_lock);
>  
> -char tipc_bclink_name[] = "multicast-link";
> +const char tipc_bclink_name[] = "multicast-link";
>  
>  
>  static u32 buf_seqno(struct sk_buff *buf)
> @@ -790,7 +790,7 @@ int tipc_bclink_init(void)
>  	INIT_LIST_HEAD(&bcbearer->bearer.cong_links);
>  	bcbearer->bearer.media = &bcbearer->media;
>  	bcbearer->media.send_msg = tipc_bcbearer_send;
> -	sprintf(bcbearer->media.name, "tipc-multicast");
> +	strcpy(bcbearer->media.name, "tipc-multicast");
>  
>  	bcl = &bclink->link;
>  	memset(bclink, 0, sizeof(struct bclink));
> @@ -802,7 +802,7 @@ int tipc_bclink_init(void)
>  	tipc_link_set_queue_limits(bcl, BCLINK_WIN_DEFAULT);
>  	bcl->b_ptr = &bcbearer->bearer;
>  	bcl->state = WORKING_WORKING;
> -	sprintf(bcl->name, tipc_bclink_name);
> +	strcpy(bcl->name, tipc_bclink_name);
>  
>  	if (BCLINK_LOG_BUF_SIZE) {
>  		char *pb = kmalloc(BCLINK_LOG_BUF_SIZE, GFP_ATOMIC);
> diff --git a/net/tipc/bcast.h b/net/tipc/bcast.h
> --- a/net/tipc/bcast.h
> +++ b/net/tipc/bcast.h
> @@ -70,7 +70,7 @@ struct port_list {
>  
>  struct node;
>  
> -extern char tipc_bclink_name[];
> +extern const char tipc_bclink_name[];
>  
>  
>  /**
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -667,7 +667,7 @@ struct sk_buff *tipc_node_get_links(cons
>          link_info.dest = tipc_own_addr & 0xfffff00;
>  	link_info.dest = htonl(link_info.dest);
>          link_info.up = htonl(1);
> -        sprintf(link_info.str, tipc_bclink_name);
> +        strcpy(link_info.str, tipc_bclink_name);
>  	tipc_cfg_append_tlv(buf, TIPC_TLV_LINK_INFO, &link_info, sizeof(link_info));
>  
>  	/* Add TLVs for any other links in scope */
> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/kernel-janitors
> 
> 
> 
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion

[walter harms]

hi Florian,

These line
+	strcpy(bcbearer->media.name, "tipc-multicast");

i gues that means tipc_bclink_name ?
an even more secure version could be like this:

	strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name);

(in case someone ever changes the size of cbearer->media.name or tipc_bclink_name and the hope
that wchat_t will never reach the kernel)

re,
 wh




Florian Westphal wrote:
> From: Florian Westphal <fw@strlen.de>
> 
> convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> 
> ---
> 
> compile tested; diffed against davem/net-2.6.
> 
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -119,7 +119,7 @@ static struct bclink *bclink = NULL;
>  static struct link *bcl = NULL;
>  static DEFINE_SPINLOCK(bc_lock);
>  
> -char tipc_bclink_name[] = "multicast-link";
> +const char tipc_bclink_name[] = "multicast-link";
>  
>  
>  static u32 buf_seqno(struct sk_buff *buf)
> @@ -790,7 +790,7 @@ int tipc_bclink_init(void)
>  	INIT_LIST_HEAD(&bcbearer->bearer.cong_links);
>  	bcbearer->bearer.media = &bcbearer->media;
>  	bcbearer->media.send_msg = tipc_bcbearer_send;
> -	sprintf(bcbearer->media.name, "tipc-multicast");
> +	strcpy(bcbearer->media.name, "tipc-multicast");
>  
>  	bcl = &bclink->link;
>  	memset(bclink, 0, sizeof(struct bclink));
> @@ -802,7 +802,7 @@ int tipc_bclink_init(void)
>  	tipc_link_set_queue_limits(bcl, BCLINK_WIN_DEFAULT);
>  	bcl->b_ptr = &bcbearer->bearer;
>  	bcl->state = WORKING_WORKING;
> -	sprintf(bcl->name, tipc_bclink_name);
> +	strcpy(bcl->name, tipc_bclink_name);
>  
>  	if (BCLINK_LOG_BUF_SIZE) {
>  		char *pb = kmalloc(BCLINK_LOG_BUF_SIZE, GFP_ATOMIC);
> diff --git a/net/tipc/bcast.h b/net/tipc/bcast.h
> --- a/net/tipc/bcast.h
> +++ b/net/tipc/bcast.h
> @@ -70,7 +70,7 @@ struct port_list {
>  
>  struct node;
>  
> -extern char tipc_bclink_name[];
> +extern const char tipc_bclink_name[];
>  
>  
>  /**
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -667,7 +667,7 @@ struct sk_buff *tipc_node_get_links(cons
>          link_info.dest = tipc_own_addr & 0xfffff00;
>  	link_info.dest = htonl(link_info.dest);
>          link_info.up = htonl(1);
> -        sprintf(link_info.str, tipc_bclink_name);
> +        strcpy(link_info.str, tipc_bclink_name);
>  	tipc_cfg_append_tlv(buf, TIPC_TLV_LINK_INFO, &link_info, sizeof(link_info));
>  
>  	/* Add TLVs for any other links in scope */
> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/kernel-janitors
> 
> 
> 

Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion

[David Miller]

From: walter harms <wharms@bfs.de>
Date: Wed, 01 Nov 2006 22:38:26 +0100

> These line
> +	strcpy(bcbearer->media.name, "tipc-multicast");
> 
> i gues that means tipc_bclink_name ?

Why?  The original code used "tipc-multicast" which is a
different string than tipc_bclink_name which is "multicast-link".

> > -	sprintf(bcbearer->media.name, "tipc-multicast");
> > +	strcpy(bcbearer->media.name, "tipc-multicast");

If you are arguing that it should be changed, that's a different
changeset to discuss.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion

[David Miller]

From: walter harms <wharms@bfs.de>
Date: Wed, 01 Nov 2006 22:38:26 +0100

> These line
> +	strcpy(bcbearer->media.name, "tipc-multicast");
> 
> i gues that means tipc_bclink_name ?

Why?  The original code used "tipc-multicast" which is a
different string than tipc_bclink_name which is "multicast-link".

> > -	sprintf(bcbearer->media.name, "tipc-multicast");
> > +	strcpy(bcbearer->media.name, "tipc-multicast");

If you are arguing that it should be changed, that's a different
changeset to discuss.

Re: [KJ] [tipc-discussion] [patch] net/tipc: sprintf/strcpy

[Florian Westphal]

walter harms <wharms@bfs.de> wrote:
> These line
>+       strcpy(bcbearer->media.name, "tipc-multicast");
> i gues that means tipc_bclink_name ?

The idea was to change how things are done, not _what_ is being done.

> an even more secure version could be like this:
> 
> 	strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name);

Ugh, please, no. The size of src is known in all cases; there is
absoluty no point in using str(n|l)cpy here.

> (in case someone ever changes the size of cbearer->media.name or tipc_bclink_name and the hope
> that wchat_t will never reach the kernel)

In this case 'someone' should be really hurt, don't you think?

Florian
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [tipc-discussion] [KJ] [patch] net/tipc: sprintf/strcpy conversion

[Florian Westphal]

walter harms <wharms@bfs.de> wrote:
> These line
>+       strcpy(bcbearer->media.name, "tipc-multicast");
> i gues that means tipc_bclink_name ?

The idea was to change how things are done, not _what_ is being done.

> an even more secure version could be like this:
> 
> 	strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name);

Ugh, please, no. The size of src is known in all cases; there is
absoluty no point in using str(n|l)cpy here.

> (in case someone ever changes the size of cbearer->media.name or tipc_bclink_name and the hope
> that wchat_t will never reach the kernel)

In this case 'someone' should be really hurt, don't you think?

Florian

Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion

[walter harms]

David Miller wrote:
> From: walter harms <wharms@bfs.de>
> Date: Wed, 01 Nov 2006 22:38:26 +0100
> 
>> These line
>> +	strcpy(bcbearer->media.name, "tipc-multicast");
>>
>> i gues that means tipc_bclink_name ?

mea culpa, i should not write mail when tired.


> 
> Why?  The original code used "tipc-multicast" which is a
> different string than tipc_bclink_name which is "multicast-link".
> 
>>> -	sprintf(bcbearer->media.name, "tipc-multicast");
>>> +	strcpy(bcbearer->media.name, "tipc-multicast");
> 
> If you are arguing that it should be changed, that's a different
> changeset to discuss.
> 
> 
> 
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion

[walter harms]

David Miller wrote:
> From: walter harms <wharms@bfs.de>
> Date: Wed, 01 Nov 2006 22:38:26 +0100
> 
>> These line
>> +	strcpy(bcbearer->media.name, "tipc-multicast");
>>
>> i gues that means tipc_bclink_name ?

mea culpa, i should not write mail when tired.


> 
> Why?  The original code used "tipc-multicast" which is a
> different string than tipc_bclink_name which is "multicast-link".
> 
>>> -	sprintf(bcbearer->media.name, "tipc-multicast");
>>> +	strcpy(bcbearer->media.name, "tipc-multicast");
> 
> If you are arguing that it should be changed, that's a different
> changeset to discuss.
> 
> 
> 

Re: [KJ] [tipc-discussion] [patch] net/tipc:

[walter harms]

Florian Westphal wrote:
> walter harms <wharms@bfs.de> wrote:
>> These line
>> +       strcpy(bcbearer->media.name, "tipc-multicast");
>> i gues that means tipc_bclink_name ?
> 
> The idea was to change how things are done, not _what_ is being done.
> 
>> an even more secure version could be like this:
>>
>> 	strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name);
> 
> Ugh, please, no. The size of src is known in all cases; there is
> absoluty no point in using str(n|l)cpy here.
> 
>> (in case someone ever changes the size of cbearer->media.name or tipc_bclink_name and the hope
>> that wchat_t will never reach the kernel)
> 
> In this case 'someone' should be really hurt, don't you think?
> 

hi florian,
i am on the side of error, the code increase is marginal and the speed penalty also, so why not ?
you make sure that an overflow may never happen, and the rest in name gets zeroed.

The problem is that when the error occurs it may be later than the actual changeset.
NTL it is an hint, and if you feel ok with it and the maintainer has no objects i have no problems either.

re,
 wh


_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [tipc-discussion] [patch] net/tipc: sprintf/strcpy conversion

[walter harms]

Florian Westphal wrote:
> walter harms <wharms@bfs.de> wrote:
>> These line
>> +       strcpy(bcbearer->media.name, "tipc-multicast");
>> i gues that means tipc_bclink_name ?
> 
> The idea was to change how things are done, not _what_ is being done.
> 
>> an even more secure version could be like this:
>>
>> 	strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name);
> 
> Ugh, please, no. The size of src is known in all cases; there is
> absoluty no point in using str(n|l)cpy here.
> 
>> (in case someone ever changes the size of cbearer->media.name or tipc_bclink_name and the hope
>> that wchat_t will never reach the kernel)
> 
> In this case 'someone' should be really hurt, don't you think?
> 

hi florian,
i am on the side of error, the code increase is marginal and the speed penalty also, so why not ?
you make sure that an overflow may never happen, and the rest in name gets zeroed.

The problem is that when the error occurs it may be later than the actual changeset.
NTL it is an hint, and if you feel ok with it and the maintainer has no objects i have no problems either.

re,
 wh

Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion

[Alexey Dobriyan]

On Wed, Nov 01, 2006 at 03:06:24PM +0100, Florian Westphal wrote:
> convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.

Ahhh, I missed the start of threads.

Patch is useless because it changes one unbounded string function into
another unbounded string function.

> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -790,7 +790,7 @@ int tipc_bclink_init(void)
>  	INIT_LIST_HEAD(&bcbearer->bearer.cong_links);
>  	bcbearer->bearer.media = &bcbearer->media;
>  	bcbearer->media.send_msg = tipc_bcbearer_send;
> -	sprintf(bcbearer->media.name, "tipc-multicast");
> +	strcpy(bcbearer->media.name, "tipc-multicast");
>
>  	bcl = &bclink->link;
>  	memset(bclink, 0, sizeof(struct bclink));
> @@ -802,7 +802,7 @@ int tipc_bclink_init(void)
>  	tipc_link_set_queue_limits(bcl, BCLINK_WIN_DEFAULT);
>  	bcl->b_ptr = &bcbearer->bearer;
>  	bcl->state = WORKING_WORKING;
> -	sprintf(bcl->name, tipc_bclink_name);
> +	strcpy(bcl->name, tipc_bclink_name);
>
>  	if (BCLINK_LOG_BUF_SIZE) {
>  		char *pb = kmalloc(BCLINK_LOG_BUF_SIZE, GFP_ATOMIC);
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -667,7 +667,7 @@ struct sk_buff *tipc_node_get_links(cons
>          link_info.dest = tipc_own_addr & 0xfffff00;
>  	link_info.dest = htonl(link_info.dest);
>          link_info.up = htonl(1);
> -        sprintf(link_info.str, tipc_bclink_name);
> +        strcpy(link_info.str, tipc_bclink_name);

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion

[Alexey Dobriyan]

On Wed, Nov 01, 2006 at 03:06:24PM +0100, Florian Westphal wrote:
> convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.

Ahhh, I missed the start of threads.

Patch is useless because it changes one unbounded string function into
another unbounded string function.

> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -790,7 +790,7 @@ int tipc_bclink_init(void)
>  	INIT_LIST_HEAD(&bcbearer->bearer.cong_links);
>  	bcbearer->bearer.media = &bcbearer->media;
>  	bcbearer->media.send_msg = tipc_bcbearer_send;
> -	sprintf(bcbearer->media.name, "tipc-multicast");
> +	strcpy(bcbearer->media.name, "tipc-multicast");
>
>  	bcl = &bclink->link;
>  	memset(bclink, 0, sizeof(struct bclink));
> @@ -802,7 +802,7 @@ int tipc_bclink_init(void)
>  	tipc_link_set_queue_limits(bcl, BCLINK_WIN_DEFAULT);
>  	bcl->b_ptr = &bcbearer->bearer;
>  	bcl->state = WORKING_WORKING;
> -	sprintf(bcl->name, tipc_bclink_name);
> +	strcpy(bcl->name, tipc_bclink_name);
>
>  	if (BCLINK_LOG_BUF_SIZE) {
>  		char *pb = kmalloc(BCLINK_LOG_BUF_SIZE, GFP_ATOMIC);
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -667,7 +667,7 @@ struct sk_buff *tipc_node_get_links(cons
>          link_info.dest = tipc_own_addr & 0xfffff00;
>  	link_info.dest = htonl(link_info.dest);
>          link_info.up = htonl(1);
> -        sprintf(link_info.str, tipc_bclink_name);
> +        strcpy(link_info.str, tipc_bclink_name);

Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion

[Hagen Paul Pfeifer]

* Alexey Dobriyan | 2006-11-03 03:09:05 [+0300]:

>On Wed, Nov 01, 2006 at 03:06:24PM +0100, Florian Westphal wrote:
>> convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.
>
>Ahhh, I missed the start of threads.
>
>Patch is useless because it changes one unbounded string function into
>another unbounded string function.

The discussion in this thread is really back-breaking!

1. To make tipc_bclink_name const there is absolutly no objection
2. Replace sprintf with strcpy

  a) First of all: If you _copy_ a string then use also strCPY()
     Thats a question of good style!

  b) If the compiler is smart enough, he realize that you want to copy 
     a string and replace the sprintf call with a
        pushl   %ebx
        call    strcpy
     Surprise - Surprise!

     Assumed you use gcc with -Os or -O2! Don't know how icc handle this 
     case. If you compile without optimization you save at least a
     "repz movsb %ds:(%esi),%es:(%edi)" instruction.

   c) Last but not least I read all the time this patch doesn't introduce
      bounds-checking. This isn't a argument because the author is aware of
      the destination length of the buffer. BTW: grep for (sprintf|strcpy) in
      /usr/src/linux and be surprised how unsecure the kernel is (thats
      ironical).

This patch is 100% OK!

HGN



_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion

[Hagen Paul Pfeifer]

* Alexey Dobriyan | 2006-11-03 03:09:05 [+0300]:

>On Wed, Nov 01, 2006 at 03:06:24PM +0100, Florian Westphal wrote:
>> convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.
>
>Ahhh, I missed the start of threads.
>
>Patch is useless because it changes one unbounded string function into
>another unbounded string function.

The discussion in this thread is really back-breaking!

1. To make tipc_bclink_name const there is absolutly no objection
2. Replace sprintf with strcpy

  a) First of all: If you _copy_ a string then use also strCPY()
     Thats a question of good style!

  b) If the compiler is smart enough, he realize that you want to copy 
     a string and replace the sprintf call with a
        pushl   %ebx
        call    strcpy
     Surprise - Surprise!

     Assumed you use gcc with -Os or -O2! Don't know how icc handle this 
     case. If you compile without optimization you save at least a
     "repz movsb %ds:(%esi),%es:(%edi)" instruction.

   c) Last but not least I read all the time this patch doesn't introduce
      bounds-checking. This isn't a argument because the author is aware of
      the destination length of the buffer. BTW: grep for (sprintf|strcpy) in
      /usr/src/linux and be surprised how unsecure the kernel is (thats
      ironical).

This patch is 100% OK!

HGN

Re: [KJ] [tipc-discussion] [patch] net/tipc:sprintf/strcpy

[Randy Dunlap]

On Thu, 2 Nov 2006 13:23:09 -0800 Horvath, Elmer wrote:

> Hi, 
> 
> Good observations.  My comments in <Elmer> </Elmer> way down below.
> 
> What is 'kernel-janitors@lists.osdl.org' ?

Mailing list for kernel-janitors work.
See http://www.kerneljanitors.org/ for info.


> <Elmer> If you want to be 'secure', as you say, then strlcpy() is a
> better choice than strncpy() to guarantee that you will have a
> null-terminated string in the destination address (as far as I
> understand the subtle differences).  strncpy() will result in a non-null
> terminated octet-string if the source string is too long.  Any future
> string operations may cause problems that are harder to trace.
> 
> Is strlcpy() available on all implementations?  I once compiled code on
> an architecture that did not support it and had to use strncpy() with an
> explicit assignment bloating code:
>     dest_string[last_octet] = '\0';
> </Elmer>

(1-2 minutes of grep:)

Yes, strlcpy() is available on all Linux architectures.
An arch. can provide a specific version of it and also
#define __HAVE_ARCH_STRLCPY

If that's not done, then the generic version in lib/string.c
will be used.

---
~Randy
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors