Fix gcc-wall warnings about strncpy

Message ID 72H9kav0twfAapoNgg-Q4eYD1U-oNt8RCfc27ftyekmZTePdhyHG8hnFoxrzBRmhXkwkhJkxa9_xNp_tV9Y2yYhKHuWbHsGb9Uc-wmgAEYM=@protonmail.ch
State New
Headers show
Series
  • Fix gcc-wall warnings about strncpy
Related show

Commit Message

ykp@protonmail.ch Aug. 15, 2018, 1:46 p.m.
From 12109693995386c9941129e65078a4305e72936e Mon Sep 17 00:00:00 2001
From: Vladyslav Tsilytskyi <ykp@protonmail.ch>
Date: Wed, 15 Aug 2018 15:25:24 +0200
Subject: [PATCH] Fix -Wsizeof-pointer-memaccess warnings

strncpy's last parameter was wrong - there was a length
of source. In order to use function correctly (and prevent
buffer overflows) last argument should denote destination's
buffer capacity.

Signed-off-by: Vladyslav Tsilytskyi <ykp@protonmail.ch>
---
 lib/e2p/ls.c            | 10 ++++------
 lib/support/plausible.c |  3 +--
 misc/mke2fs.c           |  2 +-
 3 files changed, 6 insertions(+), 9 deletions(-)

--
2.17.1

Comments

Theodore Y. Ts'o Aug. 16, 2018, 3:26 a.m. | #1
On Wed, Aug 15, 2018 at 01:46:37PM +0000, ykp@protonmail.ch wrote:
> From 12109693995386c9941129e65078a4305e72936e Mon Sep 17 00:00:00 2001
> From: Vladyslav Tsilytskyi <ykp@protonmail.ch>
> Date: Wed, 15 Aug 2018 15:25:24 +0200
> Subject: [PATCH] Fix -Wsizeof-pointer-memaccess warnings
> 
> strncpy's last parameter was wrong - there was a length
> of source. In order to use function correctly (and prevent
> buffer overflows) last argument should denote destination's
> buffer capacity.

That's actually not a bug.  It's deliberate.  That's because the
superblock fields are fixed-length char arrays which are null filled,
but if s_volume_name is a 16 byte character array --- and it's legal
for a 16 byte volume label to completely fill s_volume_name,t in which
case s_volume_name is ***NOT*** null filled.

So what we do is make sure buf[] is larger than the superblock field,
zero-fill it, and use strncpy with the 3rd parameter set to size of
the source.  For example:

	if (sb->s_volume_name[0]) {
		memset(buf, 0, sizeof(buf));
		strncpy(buf, sb->s_volume_name, sizeof(sb->s_volume_name));
		strncpy(buf, sb->s_volume_name, sizeof(buf));
	} else
		strcpy(buf, "<none>");

Not all GCC warnings are valid; you can't blindly assuming they are
valid.  That's why they are warnings.  :-)

Cheers,

					- Ted
ykp@protonmail.ch Aug. 17, 2018, 10:02 p.m. | #2
Ted,

> That's actually not a bug. It's deliberate.

If there is such misleading code, why don't we at least comment it?
Just tell developers: "that is intentional, don't waste your time".

Another type of errors that gcc complains about is [-Wsign-compare].
I'm not sure if it's a good solution to make ltype of type int
in structure, but this is definitely the shortest solution.

diff --git a/lib/ext2fs/tdb.c b/lib/ext2fs/tdb.c
index 195a4c0b..baf35913 100644
--- a/lib/ext2fs/tdb.c
+++ b/lib/ext2fs/tdb.c
@@ -202,7 +202,7 @@ struct tdb_header {
 struct tdb_lock_type {
        int list;
        u32 count;
-       u32 ltype;
+       int ltype;
 };

 struct tdb_traverse_lock {
@@ -855,7 +855,7 @@ static int tdb_oob(struct tdb_context *tdb, tdb_off_t len, int probe)
                return TDB_ERRCODE(TDB_ERR_IO, -1);
        }

-       if (st.st_size < (size_t)len) {
+       if (st.st_size < (__off_t)len) {
                if (!probe) {
                        /* Ensure ecode is set for log fn. */
                        tdb->ecode = TDB_ERR_IO;
@@ -3008,7 +3008,7 @@ static int tdb_dump_chain(struct tdb_context *tdb, int i)
 void tdb_dump_all(struct tdb_context *tdb)
 {
        int i;
-       for (i=0;i<tdb->header.hash_size;i++) {
+       for (i=0; (unsigned int)i < tdb->header.hash_size; i++) {
                tdb_dump_chain(tdb, i);
        }
        printf("freelist:\n");

These are not bugs, but such change will reduce number of warnings.

Regards,
Vlad

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On August 16, 2018 5:26 AM, Theodore Y. Ts'o tytso@mit.edu wrote:

> On Wed, Aug 15, 2018 at 01:46:37PM +0000, ykp@protonmail.ch wrote:
>
> > From 12109693995386c9941129e65078a4305e72936e Mon Sep 17 00:00:00 2001
> > From: Vladyslav Tsilytskyi ykp@protonmail.ch
> > Date: Wed, 15 Aug 2018 15:25:24 +0200
> > Subject: [PATCH] Fix -Wsizeof-pointer-memaccess warnings
> > strncpy's last parameter was wrong - there was a length
> > of source. In order to use function correctly (and prevent
> > buffer overflows) last argument should denote destination's
> > buffer capacity.
>
> That's actually not a bug. It's deliberate. That's because the
> superblock fields are fixed-length char arrays which are null filled,
> but if s_volume_name is a 16 byte character array --- and it's legal
> for a 16 byte volume label to completely fill s_volume_name,t in which
> case s_volume_name is NOT null filled.
> So what we do is make sure buf[] is larger than the superblock field,
> zero-fill it, and use strncpy with the 3rd parameter set to size of
> the source. For example:
> if (sb->s_volume_name[0]) {
>
>     	memset(buf, 0, sizeof(buf));
>     	strncpy(buf, sb->s_volume_name, sizeof(sb->s_volume_name));
>
>     	strncpy(buf, sb->s_volume_name, sizeof(buf));
>
>     } else
>     	strcpy(buf, "<none>");
>
>
> Not all GCC warnings are valid; you can't blindly assuming they are
> valid. That's why they are warnings. :-)
> Cheers,
>
> -   Ted

Patch

diff --git a/lib/e2p/ls.c b/lib/e2p/ls.c
index a7586e09..32ced7f2 100644
--- a/lib/e2p/ls.c
+++ b/lib/e2p/ls.c
@@ -234,13 +234,13 @@  void list_super2(struct ext2_super_block * sb, FILE *f)
 				  EXT2_BLOCK_SIZE(sb));
 	if (sb->s_volume_name[0]) {
 		memset(buf, 0, sizeof(buf));
-		strncpy(buf, sb->s_volume_name, sizeof(sb->s_volume_name));
+		strncpy(buf, sb->s_volume_name, sizeof(buf));
 	} else
 		strcpy(buf, "<none>");
 	fprintf(f, "Filesystem volume name:   %s\n", buf);
 	if (sb->s_last_mounted[0]) {
 		memset(buf, 0, sizeof(buf));
-		strncpy(buf, sb->s_last_mounted, sizeof(sb->s_last_mounted));
+		strncpy(buf, sb->s_last_mounted, sizeof(buf));
 	} else
 		strcpy(buf, "<not available>");
 	fprintf(f, "Last mounted on:          %s\n", buf);
@@ -420,8 +420,7 @@  void list_super2(struct ext2_super_block * sb, FILE *f)
 		tm = sb->s_first_error_time;
 		fprintf(f, "First error time:         %s", ctime(&tm));
 		memset(buf, 0, sizeof(buf));
-		strncpy(buf, (char *)sb->s_first_error_func,
-			sizeof(sb->s_first_error_func));
+		strncpy(buf, (char *)sb->s_first_error_func, sizeof(buf));
 		fprintf(f, "First error function:     %s\n", buf);
 		fprintf(f, "First error line #:       %u\n",
 			sb->s_first_error_line);
@@ -434,8 +433,7 @@  void list_super2(struct ext2_super_block * sb, FILE *f)
 		tm = sb->s_last_error_time;
 		fprintf(f, "Last error time:          %s", ctime(&tm));
 		memset(buf, 0, sizeof(buf));
-		strncpy(buf, (char *)sb->s_last_error_func,
-			sizeof(sb->s_last_error_func));
+		strncpy(buf, (char *)sb->s_last_error_func, sizeof(buf));
 		fprintf(f, "Last error function:      %s\n", buf);
 		fprintf(f, "Last error line #:        %u\n",
 			sb->s_last_error_line);
diff --git a/lib/support/plausible.c b/lib/support/plausible.c
index a7268980..6a6da61f 100644
--- a/lib/support/plausible.c
+++ b/lib/support/plausible.c
@@ -113,8 +113,7 @@  static void print_ext2_info(const char *device)
 		tm = sb->s_mtime;
 		if (sb->s_last_mounted[0]) {
 			memset(buf, 0, sizeof(buf));
-			strncpy(buf, sb->s_last_mounted,
-				sizeof(sb->s_last_mounted));
+			strncpy(buf, sb->s_last_mounted, sizeof(buf));
 			printf(_("\tlast mounted on %s on %s"), buf,
 			       ctime(&tm));
 		} else
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index b23ea766..2afe91cc 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -674,7 +674,7 @@  static void show_stats(ext2_filsys fs)
 		       ext2fs_blocks_count(&fs_param) - ext2fs_blocks_count(s));

 	memset(buf, 0, sizeof(buf));
-	strncpy(buf, s->s_volume_name, sizeof(s->s_volume_name));
+	strncpy(buf, s->s_volume_name, sizeof(buf));
 	printf(_("Filesystem label=%s\n"), buf);
 	os = e2p_os2string(fs->super->s_creator_os);
 	if (os)