[PATCH 0/3] mdadm: Fix some building errors

[PATCH 0/3] mdadm: Fix some building errors

[Xiao Ni]

There are some error buildings in Fedora release 26 (Rawhide)
The gcc version is gcc (GCC) 7.0.1 20170211 (Red Hat 7.0.1-0.8)

There are three types of errors:
1. Fall through 
mdadm.c:149:28: error: this statement may fall through [-Werror=implicit-fallthrough=]
    if (mode == ASSEMBLE || mode == BUILD ||
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        mode == CREATE || mode == GROW ||
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        mode == INCREMENTAL || mode == MANAGE)
        ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
mdadm.c:151:3: note: here
   case Brief:
   ^~~~
2. format-overflow
Detail.c: In function ‘Detail’:
Detail.c:584:31: error: ‘%s’ directive writing up to 255 bytes into a region of size 189 [-Werror=format-overflow=]
     sprintf(path, "/sys/block/%s/md/metadata_version",
                               ^~
Detail.c:584:5: note: ‘sprintf’ output between 32 and 287 bytes into a destination of size 200
     sprintf(path, "/sys/block/%s/md/metadata_version",
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      de->d_name);
      ~~~~~~~~~~~
3. format-truncation
super-intel.c: In function ‘examine_super_imsm’:
super-intel.c:1815:30: error: ‘%s’ directive output may be truncated writing up to 31 bytes into a region of size 24 [-Werror=format-truncation=]
  snprintf(str, MPB_SIG_LEN, "%s", mpb->sig);
                              ^~
super-intel.c:1815:2: note: ‘snprintf’ output between 1 and 32 bytes into a destination of size 24
  snprintf(str, MPB_SIG_LEN, "%s", mpb->sig);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Xiao Ni (3):
  Replace snprintf with strncpy at some places to avoid truncation
  Add Wimplicit-fallthrough=0 in Makefile
  Specify enough length when write to buffer

 Detail.c      |  2 +-
 Makefile      |  5 +++++
 super-intel.c | 11 +++++++----
 super0.c      |  2 +-
 util.c        |  2 +-
 5 files changed, 15 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH 1/3] mdadm: Replace snprintf with strncpy at some places to avoid truncation

[Xiao Ni]

In gcc7 there are some building errors like:
directive output may be truncated writing up to 31 bytes into a region of size 24
snprintf(str, MPB_SIG_LEN, %s, mpb->sig);

It just need to copy one string to target. So use strncpy to replace it.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 super-intel.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index d5e9517..e1618f1 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1811,7 +1811,8 @@ static void examine_super_imsm(struct supertype *st, char *homehost)
 	__u32 reserved = imsm_reserved_sectors(super, super->disks);
 	struct dl *dl;
 
-	snprintf(str, MPB_SIG_LEN, "%s", mpb->sig);
+	strncpy(str, (char *)mpb->sig, MPB_SIG_LEN);
+	str[MPB_SIG_LEN-1] = '\0';
 	printf("          Magic : %s\n", str);
 	snprintf(str, strlen(MPB_VERSION_RAID0), "%s", get_imsm_version(mpb));
 	printf("        Version : %s\n", get_imsm_version(mpb));
@@ -5227,7 +5228,7 @@ static int init_super_imsm_volume(struct supertype *st, mdu_array_info_t *info,
 			disk->status = CONFIGURED_DISK | FAILED_DISK;
 			disk->scsi_id = __cpu_to_le32(~(__u32)0);
 			snprintf((char *) disk->serial, MAX_RAID_SERIAL_LEN,
-				 "missing:%d", i);
+				 "missing:%d", (__u8)i);
 		}
 		find_missing(super);
 	} else {
@@ -7142,14 +7143,16 @@ static int update_subarray_imsm(struct supertype *st, char *subarray,
 
 			u->type = update_rename_array;
 			u->dev_idx = vol;
-			snprintf((char *) u->name, MAX_RAID_SERIAL_LEN, "%s", name);
+			strncpy((char *) u->name, name, MAX_RAID_SERIAL_LEN);
+			u->name[MAX_RAID_SERIAL_LEN-1] = '\0';
 			append_metadata_update(st, u, sizeof(*u));
 		} else {
 			struct imsm_dev *dev;
 			int i;
 
 			dev = get_imsm_dev(super, vol);
-			snprintf((char *) dev->volume, MAX_RAID_SERIAL_LEN, "%s", name);
+			strncpy((char *) dev->volume, name, MAX_RAID_SERIAL_LEN);
+			dev->volume[MAX_RAID_SERIAL_LEN-1] = '\0';
 			for (i = 0; i < mpb->num_raid_devs; i++) {
 				dev = get_imsm_dev(super, i);
 				handle_missing(super, dev);
-- 
2.7.4

[PATCH 2/3] mdadm: Add Wimplicit-fallthrough=0 in Makefile

[Xiao Ni]

There are many errors like 'error: this statement may fall through'.
But the logic is right. So add the flag Wimplicit-fallthrough=0
to disable the error messages. The method I use is from
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
#index-Wimplicit-fallthrough-375

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Makefile | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Makefile b/Makefile
index a6f464c..d1a6ac4 100644
--- a/Makefile
+++ b/Makefile
@@ -48,6 +48,11 @@ ifdef WARN_UNUSED
 CWFLAGS += -Wp,-D_FORTIFY_SOURCE=2 -O3
 endif
 
+FALLTHROUGH := $(shell gcc -v --help 2>&1 | grep "implicit-fallthrough" | wc -l)
+ifneq "$(FALLTHROUGH)"  "0"
+CWFLAGS += -Wimplicit-fallthrough=0
+endif
+
 ifdef DEBIAN
 CPPFLAGS += -DDEBIAN
 endif
-- 
2.7.4

[PATCH 3/3] mdadm: Specify enough length when write to buffer

[Xiao Ni]

In Detail.c the buffer path in function Detail is defined as path[200],
in fact the max lenth of content which needs to write to the buffer is
287. Because the length of dname of struct dirent is 255.
During building it reports error:
error: ‘%s’ directive writing up to 255 bytes into a region of size 189
[-Werror=format-overflow=]

In function examine_super0 there is a buffer nb with length 5.
But it need to show a int type argument. The lenght of max
number of int is 10. So the buffer length should be 11.

In human_size function the length of buf is 30. During building
there is a error:
output between 20 and 47 bytes into a destination of size 30.
Change the length to 47.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Detail.c | 2 +-
 super0.c | 2 +-
 util.c   | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Detail.c b/Detail.c
index 509b0d4..cb33794 100644
--- a/Detail.c
+++ b/Detail.c
@@ -575,7 +575,7 @@ This is pretty boring
 			printf("  Member Arrays :");
 
 			while (dir && (de = readdir(dir)) != NULL) {
-				char path[200];
+				char path[287];
 				char vbuf[1024];
 				int nlen = strlen(sra->sys_name);
 				dev_t devid;
diff --git a/super0.c b/super0.c
index 938cfd9..f5b4507 100644
--- a/super0.c
+++ b/super0.c
@@ -231,7 +231,7 @@ static void examine_super0(struct supertype *st, char *homehost)
 	     d++) {
 		mdp_disk_t *dp;
 		char *dv;
-		char nb[5];
+		char nb[11];
 		int wonly, failfast;
 		if (d>=0) dp = &sb->disks[d];
 		else dp = &sb->this_disk;
diff --git a/util.c b/util.c
index f100972..32bd909 100644
--- a/util.c
+++ b/util.c
@@ -811,7 +811,7 @@ unsigned long calc_csum(void *super, int bytes)
 #ifndef MDASSEMBLE
 char *human_size(long long bytes)
 {
-	static char buf[30];
+	static char buf[47];
 
 	/* We convert bytes to either centi-M{ega,ibi}bytes or
 	 * centi-G{igi,ibi}bytes, with appropriate rounding,
-- 
2.7.4

[PATCH 1/3] mdadm: Replace snprintf with strncpy at some places to avoid truncation

[Xiao Ni]

In gcc7 there are some building errors like:
directive output may be truncated writing up to 31 bytes into a region of size 24
snprintf(str, MPB_SIG_LEN, %s, mpb->sig);

It just need to copy one string to target. So use strncpy to replace it.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 super-intel.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index d5e9517..e1618f1 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1811,7 +1811,8 @@ static void examine_super_imsm(struct supertype *st, char *homehost)
 	__u32 reserved = imsm_reserved_sectors(super, super->disks);
 	struct dl *dl;
 
-	snprintf(str, MPB_SIG_LEN, "%s", mpb->sig);
+	strncpy(str, (char *)mpb->sig, MPB_SIG_LEN);
+	str[MPB_SIG_LEN-1] = '\0';
 	printf("          Magic : %s\n", str);
 	snprintf(str, strlen(MPB_VERSION_RAID0), "%s", get_imsm_version(mpb));
 	printf("        Version : %s\n", get_imsm_version(mpb));
@@ -5227,7 +5228,7 @@ static int init_super_imsm_volume(struct supertype *st, mdu_array_info_t *info,
 			disk->status = CONFIGURED_DISK | FAILED_DISK;
 			disk->scsi_id = __cpu_to_le32(~(__u32)0);
 			snprintf((char *) disk->serial, MAX_RAID_SERIAL_LEN,
-				 "missing:%d", i);
+				 "missing:%d", (__u8)i);
 		}
 		find_missing(super);
 	} else {
@@ -7142,14 +7143,16 @@ static int update_subarray_imsm(struct supertype *st, char *subarray,
 
 			u->type = update_rename_array;
 			u->dev_idx = vol;
-			snprintf((char *) u->name, MAX_RAID_SERIAL_LEN, "%s", name);
+			strncpy((char *) u->name, name, MAX_RAID_SERIAL_LEN);
+			u->name[MAX_RAID_SERIAL_LEN-1] = '\0';
 			append_metadata_update(st, u, sizeof(*u));
 		} else {
 			struct imsm_dev *dev;
 			int i;
 
 			dev = get_imsm_dev(super, vol);
-			snprintf((char *) dev->volume, MAX_RAID_SERIAL_LEN, "%s", name);
+			strncpy((char *) dev->volume, name, MAX_RAID_SERIAL_LEN);
+			dev->volume[MAX_RAID_SERIAL_LEN-1] = '\0';
 			for (i = 0; i < mpb->num_raid_devs; i++) {
 				dev = get_imsm_dev(super, i);
 				handle_missing(super, dev);
-- 
2.7.4

Re: [PATCH 1/3] mdadm: Replace snprintf with strncpy at some places to avoid truncation

[jes.sorensen]

Xiao Ni <xni@redhat.com> writes:
> In gcc7 there are some building errors like:
> directive output may be truncated writing up to 31 bytes into a region of size 24
> snprintf(str, MPB_SIG_LEN, %s, mpb->sig);
>
> It just need to copy one string to target. So use strncpy to replace it.
>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  super-intel.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/super-intel.c b/super-intel.c
> index d5e9517..e1618f1 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -1811,7 +1811,8 @@ static void examine_super_imsm(struct supertype *st, char *homehost)
>  	__u32 reserved = imsm_reserved_sectors(super, super->disks);
>  	struct dl *dl;
>  
> -	snprintf(str, MPB_SIG_LEN, "%s", mpb->sig);
> +	strncpy(str, (char *)mpb->sig, MPB_SIG_LEN);
> +	str[MPB_SIG_LEN-1] = '\0';
>  	printf("          Magic : %s\n", str);
>  	snprintf(str, strlen(MPB_VERSION_RAID0), "%s", get_imsm_version(mpb));
>  	printf("        Version : %s\n", get_imsm_version(mpb));

Given str is defined as 'char str[MAX_SIGNATURE_LENGTH]', it would be
more correct to use MAX_SIGNATURE_LENGTH as the size limit here.

> @@ -5227,7 +5228,7 @@ static int init_super_imsm_volume(struct supertype *st, mdu_array_info_t *info,
>  			disk->status = CONFIGURED_DISK | FAILED_DISK;
>  			disk->scsi_id = __cpu_to_le32(~(__u32)0);
>  			snprintf((char *) disk->serial, MAX_RAID_SERIAL_LEN,
> -				 "missing:%d", i);
> +				 "missing:%d", (__u8)i);
>  		}
>  		find_missing(super);
>  	} else {

This is unrelated to the commit message.

> @@ -7142,14 +7143,16 @@ static int update_subarray_imsm(struct supertype *st, char *subarray,
>  
>  			u->type = update_rename_array;
>  			u->dev_idx = vol;
> -			snprintf((char *) u->name, MAX_RAID_SERIAL_LEN, "%s", name);
> +			strncpy((char *) u->name, name, MAX_RAID_SERIAL_LEN);
> +			u->name[MAX_RAID_SERIAL_LEN-1] = '\0';
>  			append_metadata_update(st, u, sizeof(*u));
>  		} else {
>  			struct imsm_dev *dev;
>  			int i;
>  
>  			dev = get_imsm_dev(super, vol);
> -			snprintf((char *) dev->volume, MAX_RAID_SERIAL_LEN, "%s", name);
> +			strncpy((char *) dev->volume, name, MAX_RAID_SERIAL_LEN);
> +			dev->volume[MAX_RAID_SERIAL_LEN-1] = '\0';
>  			for (i = 0; i < mpb->num_raid_devs; i++) {
>  				dev = get_imsm_dev(super, i);
>  				handle_missing(super, dev);

These two look fine.

Jes

Re: [PATCH 2/3] mdadm: Add Wimplicit-fallthrough=0 in Makefile

[jes.sorensen]

Xiao Ni <xni@redhat.com> writes:
> There are many errors like 'error: this statement may fall through'.
> But the logic is right. So add the flag Wimplicit-fallthrough=0
> to disable the error messages. The method I use is from
> https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
> #index-Wimplicit-fallthrough-375
>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  Makefile | 5 +++++
>  1 file changed, 5 insertions(+)

Applied!

Thanks,
Jes

Re: [PATCH 3/3] mdadm: Specify enough length when write to buffer

[jes.sorensen]

Xiao Ni <xni@redhat.com> writes:
> In Detail.c the buffer path in function Detail is defined as path[200],
> in fact the max lenth of content which needs to write to the buffer is
> 287. Because the length of dname of struct dirent is 255.
> During building it reports error:
> error: ‘%s’ directive writing up to 255 bytes into a region of size 189
> [-Werror=format-overflow=]
>
> In function examine_super0 there is a buffer nb with length 5.
> But it need to show a int type argument. The lenght of max
> number of int is 10. So the buffer length should be 11.
>
> In human_size function the length of buf is 30. During building
> there is a error:
> output between 20 and 47 bytes into a destination of size 30.
> Change the length to 47.
>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  Detail.c | 2 +-
>  super0.c | 2 +-
>  util.c   | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)

Applied!

Thanks!
Jes

Re: [PATCH 1/3] mdadm: Replace snprintf with strncpy at some places to avoid truncation

[Xiao Ni]

----- Original Message -----
> From: "jes sorensen" <jes.sorensen@gmail.com>
> To: "Xiao Ni" <xni@redhat.com>
> Cc: linux-raid@vger.kernel.org, ncroxon@redhat.com, "artur paszkiewicz" <artur.paszkiewicz@intel.com>
> Sent: Saturday, March 18, 2017 3:55:43 AM
> Subject: Re: [PATCH 1/3] mdadm: Replace snprintf with strncpy at some places to avoid truncation
> 
> Xiao Ni <xni@redhat.com> writes:
> > In gcc7 there are some building errors like:
> > directive output may be truncated writing up to 31 bytes into a region of
> > size 24
> > snprintf(str, MPB_SIG_LEN, %s, mpb->sig);
> >
> > It just need to copy one string to target. So use strncpy to replace it.
> >
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> >  super-intel.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/super-intel.c b/super-intel.c
> > index d5e9517..e1618f1 100644
> > --- a/super-intel.c
> > +++ b/super-intel.c
> > @@ -1811,7 +1811,8 @@ static void examine_super_imsm(struct supertype *st,
> > char *homehost)
> >  	__u32 reserved = imsm_reserved_sectors(super, super->disks);
> >  	struct dl *dl;
> >  
> > -	snprintf(str, MPB_SIG_LEN, "%s", mpb->sig);
> > +	strncpy(str, (char *)mpb->sig, MPB_SIG_LEN);
> > +	str[MPB_SIG_LEN-1] = '\0';
> >  	printf("          Magic : %s\n", str);
> >  	snprintf(str, strlen(MPB_VERSION_RAID0), "%s", get_imsm_version(mpb));
> >  	printf("        Version : %s\n", get_imsm_version(mpb));
> 
> Given str is defined as 'char str[MAX_SIGNATURE_LENGTH]', it would be
> more correct to use MAX_SIGNATURE_LENGTH as the size limit here.

I talked with Artur about this. It should use MPB_SIG_LEN, because the mpb->sig
has version after the signature. It will print more if use MAX_SIGNATURE_LENGTH.

> 
> > @@ -5227,7 +5228,7 @@ static int init_super_imsm_volume(struct supertype
> > *st, mdu_array_info_t *info,
> >  			disk->status = CONFIGURED_DISK | FAILED_DISK;
> >  			disk->scsi_id = __cpu_to_le32(~(__u32)0);
> >  			snprintf((char *) disk->serial, MAX_RAID_SERIAL_LEN,
> > -				 "missing:%d", i);
> > +				 "missing:%d", (__u8)i);
> >  		}
> >  		find_missing(super);
> >  	} else {
> 
> This is unrelated to the commit message.

I'll re-send this patch later.

Regards
Xiao
> 
> > @@ -7142,14 +7143,16 @@ static int update_subarray_imsm(struct supertype
> > *st, char *subarray,
> >  
> >  			u->type = update_rename_array;
> >  			u->dev_idx = vol;
> > -			snprintf((char *) u->name, MAX_RAID_SERIAL_LEN, "%s", name);
> > +			strncpy((char *) u->name, name, MAX_RAID_SERIAL_LEN);
> > +			u->name[MAX_RAID_SERIAL_LEN-1] = '\0';
> >  			append_metadata_update(st, u, sizeof(*u));
> >  		} else {
> >  			struct imsm_dev *dev;
> >  			int i;
> >  
> >  			dev = get_imsm_dev(super, vol);
> > -			snprintf((char *) dev->volume, MAX_RAID_SERIAL_LEN, "%s", name);
> > +			strncpy((char *) dev->volume, name, MAX_RAID_SERIAL_LEN);
> > +			dev->volume[MAX_RAID_SERIAL_LEN-1] = '\0';
> >  			for (i = 0; i < mpb->num_raid_devs; i++) {
> >  				dev = get_imsm_dev(super, i);
> >  				handle_missing(super, dev);
> 
> These two look fine.
> 
> Jes
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>