diff mbox series

Use strscpy() instead of strcpy()

Message ID 20241111221037.92853-1-abdul.rahim@myyahoo.com (mailing list archive)
State New
Headers show
Series Use strscpy() instead of strcpy() | expand

Commit Message

Abdul Rahim Nov. 11, 2024, 10:10 p.m. UTC 
strcpy() is generally considered unsafe and use of strscpy() is
recommended [1]

this fixes checkpatch warning:
    WARNING: Prefer strscpy over strcpy

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy [1]
Signed-off-by: Abdul Rahim <abdul.rahim@myyahoo.com>
---
 fs/ceph/export.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Christophe JAILLET Nov. 12, 2024, 6:21 a.m. UTC | #1 
Le 11/11/2024 à 23:10, Abdul Rahim a écrit :
> strcpy() is generally considered unsafe and use of strscpy() is
> recommended [1]
> 
> this fixes checkpatch warning:
>      WARNING: Prefer strscpy over strcpy
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy [1]
> Signed-off-by: Abdul Rahim <abdul.rahim@myyahoo.com>
> ---
>   fs/ceph/export.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/export.c b/fs/ceph/export.c
> index 44451749c544..0e5b3c7b3756 100644
> --- a/fs/ceph/export.c
> +++ b/fs/ceph/export.c
> @@ -452,7 +452,7 @@ static int __get_snap_name(struct dentry *parent, char *name,
>   		goto out;
>   	if (ceph_snap(inode) == CEPH_SNAPDIR) {
>   		if (ceph_snap(dir) == CEPH_NOSNAP) {
> -			strcpy(name, fsc->mount_options->snapdir_name);
> +			strscpy(name, fsc->mount_options->snapdir_name);

This does not compile because when the size of 'name' is not known at 
compilation time, you need to use the 3-argument version of strscpy().

Please always compile test your patches before sending them. Even, when 
the change looks trivial.

CJ

>   			err = 0;
>   		}
>   		goto out;
kernel test robot Nov. 13, 2024, 1:06 a.m. UTC | #2 
Hi Abdul,

kernel test robot noticed the following build errors:

[auto build test ERROR on ceph-client/testing]
[also build test ERROR on ceph-client/for-linus linus/master v6.12-rc7 next-20241112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Abdul-Rahim/Use-strscpy-instead-of-strcpy/20241112-064257
base:   https://github.com/ceph/ceph-client.git testing
patch link:    https://lore.kernel.org/r/20241111221037.92853-1-abdul.rahim%40myyahoo.com
patch subject: [PATCH] Use strscpy() instead of strcpy()
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20241113/202411130853.c11sinW2-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241113/202411130853.c11sinW2-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411130853.c11sinW2-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/bitfield.h:10,
                    from include/linux/fortify-string.h:5,
                    from include/linux/string.h:390,
                    from include/linux/ceph/ceph_debug.h:7,
                    from fs/ceph/export.c:2:
   fs/ceph/export.c: In function '__get_snap_name':
>> include/linux/build_bug.h:16:51: error: negative width in bit-field '<anonymous>'
      16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
         |                                                   ^
   include/linux/compiler.h:243:33: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
     243 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
         |                                 ^~~~~~~~~~~~~~~~~
   include/linux/string.h:79:47: note: in expansion of macro '__must_be_array'
      79 |         sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst) +    \
         |                                               ^~~~~~~~~~~~~~~
   include/linux/args.h:25:24: note: in expansion of macro '__strscpy0'
      25 | #define __CONCAT(a, b) a ## b
         |                        ^
   include/linux/args.h:26:27: note: in expansion of macro '__CONCAT'
      26 | #define CONCATENATE(a, b) __CONCAT(a, b)
         |                           ^~~~~~~~
   include/linux/string.h:113:9: note: in expansion of macro 'CONCATENATE'
     113 |         CONCATENATE(__strscpy, COUNT_ARGS(__VA_ARGS__))(dst, src, __VA_ARGS__)
         |         ^~~~~~~~~~~
   fs/ceph/export.c:455:25: note: in expansion of macro 'strscpy'
     455 |                         strscpy(name, fsc->mount_options->snapdir_name);
         |                         ^~~~~~~


vim +16 include/linux/build_bug.h

bc6245e5efd70c Ian Abbott       2017-07-10   6  
bc6245e5efd70c Ian Abbott       2017-07-10   7  #ifdef __CHECKER__
bc6245e5efd70c Ian Abbott       2017-07-10   8  #define BUILD_BUG_ON_ZERO(e) (0)
bc6245e5efd70c Ian Abbott       2017-07-10   9  #else /* __CHECKER__ */
bc6245e5efd70c Ian Abbott       2017-07-10  10  /*
bc6245e5efd70c Ian Abbott       2017-07-10  11   * Force a compilation error if condition is true, but also produce a
8788994376d84d Rikard Falkeborn 2019-12-04  12   * result (of value 0 and type int), so the expression can be used
bc6245e5efd70c Ian Abbott       2017-07-10  13   * e.g. in a structure initializer (or where-ever else comma expressions
bc6245e5efd70c Ian Abbott       2017-07-10  14   * aren't permitted).
bc6245e5efd70c Ian Abbott       2017-07-10  15   */
8788994376d84d Rikard Falkeborn 2019-12-04 @16  #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
527edbc18a70e7 Masahiro Yamada  2019-01-03  17  #endif /* __CHECKER__ */
527edbc18a70e7 Masahiro Yamada  2019-01-03  18
Abdul Rahim Nov. 13, 2024, 8:15 p.m. UTC | #3 
On Tue, Nov 12, 2024 at 07:21:54AM +0100, Christophe JAILLET wrote:
> Le 11/11/2024 � 23:10, Abdul Rahim a �crit�:
> > strcpy() is generally considered unsafe and use of strscpy() is
> > recommended [1]
> > 
> > this fixes checkpatch warning:
> >      WARNING: Prefer strscpy over strcpy
> > 
> > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy [1]
> > Signed-off-by: Abdul Rahim <abdul.rahim@myyahoo.com>
> > ---
> >   fs/ceph/export.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/ceph/export.c b/fs/ceph/export.c
> > index 44451749c544..0e5b3c7b3756 100644
> > --- a/fs/ceph/export.c
> > +++ b/fs/ceph/export.c
> > @@ -452,7 +452,7 @@ static int __get_snap_name(struct dentry *parent, char *name,
> >   		goto out;
> >   	if (ceph_snap(inode) == CEPH_SNAPDIR) {
> >   		if (ceph_snap(dir) == CEPH_NOSNAP) {
> > -			strcpy(name, fsc->mount_options->snapdir_name);
> > +			strscpy(name, fsc->mount_options->snapdir_name);
> 
> This does not compile because when the size of 'name' is not known at
> compilation time, you need to use the 3-argument version of strscpy().
> 
> Please always compile test your patches before sending them. Even, when the
> change looks trivial.
> 

Sure.

> CJ
> 
> >   			err = 0;
> >   		}
> >   		goto out;
> 
> 

Should it be: NAME_MAX+1 ? 

See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/exportfs.h?h=v6.12-rc7#n203
Christophe JAILLET Nov. 13, 2024, 9:28 p.m. UTC | #4 
Le 13/11/2024 à 21:15, Abdul Rahim a écrit :
> On Tue, Nov 12, 2024 at 07:21:54AM +0100, Christophe JAILLET wrote:
>> Le 11/11/2024 � 23:10, Abdul Rahim a �crit�:
>>> strcpy() is generally considered unsafe and use of strscpy() is
>>> recommended [1]
>>>
>>> this fixes checkpatch warning:
>>>       WARNING: Prefer strscpy over strcpy
>>>
>>> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy [1]
>>> Signed-off-by: Abdul Rahim <abdul.rahim@myyahoo.com>
>>> ---
>>>    fs/ceph/export.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/ceph/export.c b/fs/ceph/export.c
>>> index 44451749c544..0e5b3c7b3756 100644
>>> --- a/fs/ceph/export.c
>>> +++ b/fs/ceph/export.c
>>> @@ -452,7 +452,7 @@ static int __get_snap_name(struct dentry *parent, char *name,
>>>    		goto out;
>>>    	if (ceph_snap(inode) == CEPH_SNAPDIR) {
>>>    		if (ceph_snap(dir) == CEPH_NOSNAP) {
>>> -			strcpy(name, fsc->mount_options->snapdir_name);
>>> +			strscpy(name, fsc->mount_options->snapdir_name);
>>
>> This does not compile because when the size of 'name' is not known at
>> compilation time, you need to use the 3-argument version of strscpy().
>>
>> Please always compile test your patches before sending them. Even, when the
>> change looks trivial.
>>
> 
> Sure.
> 
>> CJ
>>
>>>    			err = 0;
>>>    		}
>>>    		goto out;
>>
>>
> 
> Should it be: NAME_MAX+1 ?
> 
> See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/exportfs.h?h=v6.12-rc7#n203
> 
> 

Looks like a good idea, maybe with a comment related to get_name() in 
the export_operations structure, so that we remind where this limit 
comes from?

CJ
Abdul Rahim Nov. 14, 2024, 9:14 a.m. UTC | #5 
On Wed, Nov 13, 2024 at 10:28:36PM +0100, Christophe JAILLET wrote:
> Le 13/11/2024 � 21:15, Abdul Rahim a �crit�:
> > On Tue, Nov 12, 2024 at 07:21:54AM +0100, Christophe JAILLET wrote:
> > > Le 11/11/2024 � 23:10, Abdul Rahim a �crit�:
> > > > strcpy() is generally considered unsafe and use of strscpy() is
> > > > recommended [1]
> > > > 
> > > > this fixes checkpatch warning:
> > > >       WARNING: Prefer strscpy over strcpy
> > > > 
> > > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy [1]
> > > > Signed-off-by: Abdul Rahim <abdul.rahim@myyahoo.com>
> > > > ---
> > > >    fs/ceph/export.c | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/ceph/export.c b/fs/ceph/export.c
> > > > index 44451749c544..0e5b3c7b3756 100644
> > > > --- a/fs/ceph/export.c
> > > > +++ b/fs/ceph/export.c
> > > > @@ -452,7 +452,7 @@ static int __get_snap_name(struct dentry *parent, char *name,
> > > >    		goto out;
> > > >    	if (ceph_snap(inode) == CEPH_SNAPDIR) {
> > > >    		if (ceph_snap(dir) == CEPH_NOSNAP) {
> > > > -			strcpy(name, fsc->mount_options->snapdir_name);
> > > > +			strscpy(name, fsc->mount_options->snapdir_name);
> > > 
> > > This does not compile because when the size of 'name' is not known at
> > > compilation time, you need to use the 3-argument version of strscpy().
> > > 
> > > Please always compile test your patches before sending them. Even, when the
> > > change looks trivial.
> > > 
> > 
> > Sure.
> > 
> > > CJ
> > > 
> > > >    			err = 0;
> > > >    		}
> > > >    		goto out;
> > > 
> > 

> > Should it be: NAME_MAX+1 ?
> > 
> > See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/exportfs.h?h=v6.12-rc7#n203
> > 
> > 
> 
> Looks like a good idea, maybe with a comment related to get_name() in the
> export_operations structure, so that we remind where this limit comes from?
> 
> CJ
> 

diff --git a/fs/ceph/export.c b/fs/ceph/export.c
index 0e5b3c7b3756..48265c879fcf 100644
--- a/fs/ceph/export.c
+++ b/fs/ceph/export.c
@@ -452,7 +452,12 @@ static int __get_snap_name(struct dentry *parent, char *name,
 		goto out;
 	if (ceph_snap(inode) == CEPH_SNAPDIR) {
 		if (ceph_snap(dir) == CEPH_NOSNAP) {
-			strcpy(name, fsc->mount_options->snapdir_name);
+			/*
+			 * get_name assumes that name is pointing to a
+			 * NAME_MAX+1 sized buffer
+			 */
+			strscpy(name, fsc->mount_options->snapdir_name, 
+					NAME_MAX+1);
 			err = 0;
 		}
 		goto out;


Looks good?
diff mbox series

Patch

diff --git a/fs/ceph/export.c b/fs/ceph/export.c
index 44451749c544..0e5b3c7b3756 100644
--- a/fs/ceph/export.c
+++ b/fs/ceph/export.c
@@ -452,7 +452,7 @@  static int __get_snap_name(struct dentry *parent, char *name,
 		goto out;
 	if (ceph_snap(inode) == CEPH_SNAPDIR) {
 		if (ceph_snap(dir) == CEPH_NOSNAP) {
-			strcpy(name, fsc->mount_options->snapdir_name);
+			strscpy(name, fsc->mount_options->snapdir_name);
 			err = 0;
 		}
 		goto out;