| Message ID | 20241111221037.92853-1-abdul.rahim@myyahoo.com |
|---|---|
| State | New |
| Headers | show |
| Series | Use strscpy() instead of strcpy() | expand |
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;
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
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
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
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?
Le 14/11/2024 à 10:14, Abdul Rahim a écrit : > On Wed, Nov 13, 2024 at 10:28:36PM +0100, Christophe JAILLET wrote: ... > 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 > + */ It is a matter of taste, and I'm not the maintainer, but my personal feeling would go for something like: /* .get_name() from struct export_operations assumes that its 'name' parameter is pointing to a NAME_MAX+1 sized buffer */ CJ > + strscpy(name, fsc->mount_options->snapdir_name, > + NAME_MAX+1); > err = 0; > } > goto out; > > > Looks good? > >
On Thu, Nov 14, 2024 at 09:53:46PM +0100, Christophe JAILLET wrote: > Le 14/11/2024 à 10:14, Abdul Rahim a écrit : > > On Wed, Nov 13, 2024 at 10:28:36PM +0100, Christophe JAILLET wrote: > > ... > > > 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 > > + */ > > It is a matter of taste, and I'm not the maintainer, but my personal feeling > would go for something like: > > /* .get_name() from struct export_operations assumes that its 'name' > parameter is pointing to a NAME_MAX+1 sized buffer */ > > CJ https://lore.kernel.org/lkml/20241115112419.11137-1-abdul.rahim@myyahoo.com/
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;
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(-)