Re: [PATCH 03/35] fbdev: sisfb: Use safer strscpy() instead of strcpy()

Re: [PATCH 03/35] fbdev: sisfb: Use safer strscpy() instead of strcpy()

[Helge Deller]

On 4/27/26 11:09, Ai Chao wrote:
> Hello David and Helge
> ...
>>>> -            strcpy(ivideo->myid, "SiS 730");
>>>> +            strscpy(ivideo->myid, "SiS 730");
>>>
>>> The compiler knows at build time the length of myid, and the "SIS 730" string.
>>> Using strscpy() has no benefit here either. Contrary, the code generated
>>> because of using strscpy() is probably even larger.
>>> Don't replace such code with strscpy().
> 
>> Both should get converted to a memcpy().
> 
>> If you increase the literal to be too long I'm pretty sure you'll
>> get a compiler warning/error from strcpy().
>> OTOH strscpy() is more likely to truncate the string (I'd need to
>> check).
> 
>> So leaving it as strcpy() is fine - and possibly even better.
>> The header files might get changed to error strcpy() unless the compiler
>> knows the source string has a constant length and the destination is
>> big enough - but that hasn't been done yet.
> 
> struct sis_video_info {
>      char    myid[40];
> }
> I have rewritten the code:
> strcpy(ivideo->myid, "SiS 730-0123456789abcdefghijklmnopqrstuvwxyz0123456789");
> Used gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04.3)
> There was no compiler warning or error.
> The strcpy copies the entire string into myid(causing a buffer overflow),

Sure it would
But the compiler issued a warning that the string is too big..
So, such places will be detected at compile time.

Helge

Re: [PATCH 03/35] fbdev: sisfb: Use safer strscpy() instead of strcpy()

[David Laight]

On Mon, 27 Apr 2026 17:09:10 +0800
Ai Chao <aichao@kylinos.cn> wrote:

> Hello David and Helge
> ...
> > > > -            strcpy(ivideo->myid, "SiS 730");
> > > > +            strscpy(ivideo->myid, "SiS 730");    
> > > 
> > > The compiler knows at build time the length of myid, and the "SIS 730" string.
> > > Using strscpy() has no benefit here either. Contrary, the code generated
> > > because of using strscpy() is probably even larger.
> > > Don't replace such code with strscpy().  
> 
> > Both should get converted to a memcpy().  
> 
> > If you increase the literal to be too long I'm pretty sure you'll
> > get a compiler warning/error from strcpy().
> > OTOH strscpy() is more likely to truncate the string (I'd need to
> > check).  
> 
> > So leaving it as strcpy() is fine - and possibly even better.
> > The header files might get changed to error strcpy() unless the compiler
> > knows the source string has a constant length and the destination is
> > big enough - but that hasn't been done yet.  
> 
> struct sis_video_info {
>     char    myid[40];
> }
> I have rewritten the code: 
> strcpy(ivideo->myid, "SiS 730-0123456789abcdefghijklmnopqrstuvwxyz0123456789");
> Used gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04.3)
> There was no compiler warning or error. 
> The strcpy copies the entire string into myid(causing a buffer overflow),
> whereas strscpy only copies 40 characters into myid according to its size.

It depends on what is in string.h and the enabled warnings.
Testing on 'godbolt' gives an error with both gcc and clang without any
special compilation options.

The linux kernel build errors strcpy() at line 799 of fortify-string.h.
strscpy() doesn't (and really shouldn't) generate an error since it is
expected to truncate overlong strings.

Since you should (at least) test compile any patches before sending them
(even trivial ones) you ought to have things setup to have checked what
happens in a kernel build.
Ideally you should also run the code.

This really means that strcpy() is better than strscpy() for copying fixed
length strings into arrays.

	David

> 
> Thanks,
> Ai Chao
> 

[PATCH 00/35] Use safer strscpy() instead of strcpy()

[Ai Chao]

This patch series introduces wrapper functions strscpy().

Use a safer function strscpy() instead of strcpy() for copying to arrays.

Only idiomatic code replacement, and no functional changes.

Summary:

  fbdev: matroxfb: Use safer strscpy() instead of strcpy()
  fbdev: i810: Use safer strscpy() instead of strcpy()
  fbdev: sisfb: Use safer strscpy() instead of strcpy()
  fbdev: geode: Use safer strscpy() instead of strcpy()
  fbdev: atafb: Use safer strscpy() instead of strcpy()
  fbdev: tdfxfb: Use safer strscpy() instead of strcpy()
  fbdev: pm2fb: Use safer strscpy() instead of strcpy()
  fbdev: xen-fbfront: Use safer strscpy() instead of strcpy()
  fbdev: controlfb: Use safer strscpy() instead of strcpy()
  fbdev: stifb: Use safer strscpy() instead of strcpy()
  fbdev: fm2fb: Use safer strscpy() instead of strcpy()
  fbdev: arkfb: Use safer strscpy() instead of strcpy()
  fbdev: vt8500lcdfb: Use safer strscpy() instead of strcpy()
  fbdev: vt8623fb: Use safer strscpy() instead of strcpy()
  fbdev: gbefb: Use safer strscpy() instead of strcpy()
  fbdev: wm8505fb: Use safer strscpy() instead of strcpy()
  fbdev: rivafb: Use safer strscpy() instead of strcpy()
  fbdev: sh7760fb: Use safer strscpy() instead of strcpy()
  fbdev: savage: Use safer strscpy() instead of strcpy()
  fbdev: atmel_lcdfb: Use safer strscpy() instead of strcpy()
  fbdev: aty128fb: Use safer strscpy() instead of strcpy()
  fbdev: amifb: Use safer strscpy() instead of strcpy()
  fbdev: s3fb: Use safer strscpy() instead of strcpy()
  fbdev: viafb: Use safer strscpy() instead of strcpy()
  fbdev: platinumfb: Use safer strscpy() instead of strcpy()
  fbdev: cyber2000fb: Use safer strscpy() instead of strcpy()
  fbdev: mb862xxfb: Use safer strscpy() instead of strcpy()
  fbdev: mmpfb: Use safer strscpy() instead of strcpy()
  fbdev: ep93xx-fb: Use safer strscpy() instead of strcpy()
  fbdev: valkyriefb: Use safer strscpy() instead of strcpy()
  fbdev: pxafb: Use safer strscpy() instead of strcpy()
  fbdev: sa1100fb: Use safer strscpy() instead of strcpy()
  fbdev: sm501fb: Use safer strscpy() instead of strcpy()
  fbdev: acornfb: Use safer strscpy() instead of strcpy()
  fbdev: ocfb: Use safer strscpy() instead of strcpy()

 drivers/video/fbdev/acornfb.c               |  2 +-
 drivers/video/fbdev/amifb.c                 |  2 +-
 drivers/video/fbdev/arkfb.c                 |  2 +-
 drivers/video/fbdev/atafb.c                 | 10 +++++-----
 drivers/video/fbdev/atmel_lcdfb.c           |  2 +-
 drivers/video/fbdev/aty/aty128fb.c          |  2 +-
 drivers/video/fbdev/controlfb.c             |  2 +-
 drivers/video/fbdev/cyber2000fb.c           |  2 +-
 drivers/video/fbdev/ep93xx-fb.c             |  2 +-
 drivers/video/fbdev/fm2fb.c                 |  2 +-
 drivers/video/fbdev/gbefb.c                 |  2 +-
 drivers/video/fbdev/geode/gx1fb_core.c      |  2 +-
 drivers/video/fbdev/geode/gxfb_core.c       |  2 +-
 drivers/video/fbdev/geode/lxfb_core.c       |  2 +-
 drivers/video/fbdev/i810/i810-i2c.c         |  2 +-
 drivers/video/fbdev/i810/i810_main.c        |  2 +-
 drivers/video/fbdev/matrox/matroxfb_base.c  |  6 +++---
 drivers/video/fbdev/matrox/matroxfb_crtc2.c |  2 +-
 drivers/video/fbdev/mb862xx/mb862xxfbdrv.c  |  2 +-
 drivers/video/fbdev/mmp/fb/mmpfb.c          |  2 +-
 drivers/video/fbdev/ocfb.c                  |  2 +-
 drivers/video/fbdev/platinumfb.c            |  2 +-
 drivers/video/fbdev/pm2fb.c                 |  6 +++---
 drivers/video/fbdev/pxafb.c                 |  2 +-
 drivers/video/fbdev/riva/rivafb-i2c.c       |  2 +-
 drivers/video/fbdev/s3fb.c                  |  2 +-
 drivers/video/fbdev/sa1100fb.c              |  2 +-
 drivers/video/fbdev/savage/savagefb-i2c.c   |  2 +-
 drivers/video/fbdev/sh7760fb.c              |  2 +-
 drivers/video/fbdev/sis/sis_main.c          | 16 ++++++++--------
 drivers/video/fbdev/sm501fb.c               |  2 +-
 drivers/video/fbdev/stifb.c                 |  2 +-
 drivers/video/fbdev/tdfxfb.c                |  6 +++---
 drivers/video/fbdev/valkyriefb.c            |  2 +-
 drivers/video/fbdev/via/viafbdev.c          |  2 +-
 drivers/video/fbdev/vt8500lcdfb.c           |  2 +-
 drivers/video/fbdev/vt8623fb.c              |  2 +-
 drivers/video/fbdev/wm8505fb.c              |  2 +-
 drivers/video/fbdev/xen-fbfront.c           |  2 +-
 39 files changed, 56 insertions(+), 56 deletions(-)

-- 
2.34.1

[PATCH 03/35] fbdev: sisfb: Use safer strscpy() instead of strcpy()

[Ai Chao]

Use a safer function strscpy() instead of strcpy() for copying to arrays.

Only idiomatic code replacement, and no functional changes.

Signed-off-by: Ai Chao <aichao@kylinos.cn>
---
 drivers/video/fbdev/sis/sis_main.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/video/fbdev/sis/sis_main.c b/drivers/video/fbdev/sis/sis_main.c
index 84567d67f71d..e87fa261f76c 100644
--- a/drivers/video/fbdev/sis/sis_main.c
+++ b/drivers/video/fbdev/sis/sis_main.c
@@ -205,7 +205,7 @@ static void sisfb_search_mode(char *name, bool quiet)
 	}
 
 	if(strlen(name) <= 19) {
-		strcpy(strbuf1, name);
+		strscpy(strbuf1, name);
 		for(i = 0; i < strlen(strbuf1); i++) {
 			if(strbuf1[i] < '0' || strbuf1[i] > '9') strbuf1[i] = ' ';
 		}
@@ -5947,33 +5947,33 @@ static int sisfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 #ifdef CONFIG_FB_SIS_300
 		case PCI_DEVICE_ID_SI_730:
 			ivideo->chip = SIS_730;
-			strcpy(ivideo->myid, "SiS 730");
+			strscpy(ivideo->myid, "SiS 730");
 			break;
 #endif
 #ifdef CONFIG_FB_SIS_315
 		case PCI_DEVICE_ID_SI_651:
 			/* ivideo->chip is ok */
-			strcpy(ivideo->myid, "SiS 651");
+			strscpy(ivideo->myid, "SiS 651");
 			break;
 		case PCI_DEVICE_ID_SI_740:
 			ivideo->chip = SIS_740;
-			strcpy(ivideo->myid, "SiS 740");
+			strscpy(ivideo->myid, "SiS 740");
 			break;
 		case PCI_DEVICE_ID_SI_661:
 			ivideo->chip = SIS_661;
-			strcpy(ivideo->myid, "SiS 661");
+			strscpy(ivideo->myid, "SiS 661");
 			break;
 		case PCI_DEVICE_ID_SI_741:
 			ivideo->chip = SIS_741;
-			strcpy(ivideo->myid, "SiS 741");
+			strscpy(ivideo->myid, "SiS 741");
 			break;
 		case PCI_DEVICE_ID_SI_760:
 			ivideo->chip = SIS_760;
-			strcpy(ivideo->myid, "SiS 760");
+			strscpy(ivideo->myid, "SiS 760");
 			break;
 		case PCI_DEVICE_ID_SI_761:
 			ivideo->chip = SIS_761;
-			strcpy(ivideo->myid, "SiS 761");
+			strscpy(ivideo->myid, "SiS 761");
 			break;
 #endif
 		default:
-- 
2.34.1

Re: [PATCH 03/35] fbdev: sisfb: Use safer strscpy() instead of strcpy()

[Helge Deller]

Hello Ai,

Thanks that you want to contribute!

But your series isn't beneficial in most areas.
Some examples:

On 4/25/26 08:58, Ai Chao wrote:
> Use a safer function strscpy() instead of strcpy() for copying to arrays.
> 
> Only idiomatic code replacement, and no functional changes.
> 
> Signed-off-by: Ai Chao <aichao@kylinos.cn>
> ---
>   drivers/video/fbdev/sis/sis_main.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/video/fbdev/sis/sis_main.c b/drivers/video/fbdev/sis/sis_main.c
> index 84567d67f71d..e87fa261f76c 100644
> --- a/drivers/video/fbdev/sis/sis_main.c
> +++ b/drivers/video/fbdev/sis/sis_main.c
> @@ -205,7 +205,7 @@ static void sisfb_search_mode(char *name, bool quiet)
>   	}
>   
>   	if(strlen(name) <= 19) {
> -		strcpy(strbuf1, name);
> +		strscpy(strbuf1, name);

We have strbuf1[20] above, and the length is checked.
There is no benefit of using strscpy() here.
(The code could be cleaned up in other ways though).

>   		for(i = 0; i < strlen(strbuf1); i++) {
>   			if(strbuf1[i] < '0' || strbuf1[i] > '9') strbuf1[i] = ' ';
>   		}
> @@ -5947,33 +5947,33 @@ static int sisfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   #ifdef CONFIG_FB_SIS_300
>   		case PCI_DEVICE_ID_SI_730:
>   			ivideo->chip = SIS_730;
> -			strcpy(ivideo->myid, "SiS 730");
> +			strscpy(ivideo->myid, "SiS 730");

The compiler knows at build time the length of myid, and the "SIS 730" string.
Using strscpy() has no benefit here either. Contrary, the code generated
because of using strscpy() is probably even larger.
Don't replace such code with strscpy().


--- a/drivers/video/fbdev/i810/i810-i2c.c
+++ b/drivers/video/fbdev/i810/i810-i2c.c
@@ -91,7 +91,7 @@ static int i810_setup_i2c_bus(struct i810fb_i2c_chan *chan, const char *name)
  {
          int rc;
  
-        strcpy(chan->adapter.name, name);
+	strscpy(chan->adapter.name, name);

Here it might make sense to use strscpy(), but it should be checked manually
and not using scripts to simply replace code.

That said: Thanks for your patches, but as-is I won't take them.

Helge

Re: [PATCH 03/35] fbdev: sisfb: Use safer strscpy() instead of strcpy()

[David Laight]

On Sat, 25 Apr 2026 10:08:08 +0200
Helge Deller <deller@gmx.de> wrote:

> Hello Ai,
...
> > -			strcpy(ivideo->myid, "SiS 730");
> > +			strscpy(ivideo->myid, "SiS 730");  
> 
> The compiler knows at build time the length of myid, and the "SIS 730" string.
> Using strscpy() has no benefit here either. Contrary, the code generated
> because of using strscpy() is probably even larger.
> Don't replace such code with strscpy().

Both should get converted to a memcpy().

If you increase the literal to be too long I'm pretty sure you'll
get a compiler warning/error from strcpy().
OTOH strscpy() is more likely to truncate the string (I'd need to
check).

So leaving it as strcpy() is fine - and possibly even better.
The header files might get changed to error strcpy() unless the compiler
knows the source string has a constant length and the destination is
big enough - but that hasn't been done yet.

	David