| Message ID | 72H9kav0twfAapoNgg-Q4eYD1U-oNt8RCfc27ftyekmZTePdhyHG8hnFoxrzBRmhXkwkhJkxa9_xNp_tV9Y2yYhKHuWbHsGb9Uc-wmgAEYM=@protonmail.ch |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix gcc-wall warnings about strncpy | expand |
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
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
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)