linux-ext4 - Re: [PATCH] Fix gcc-wall warnings about strncpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <RBNr2IVcyckyt0jM0lf3btsvhcY4EHQPSgOpjdrL8IHqMHoMW8tE0Fy43FtlkEepaw6L-EmZC3-h6ifrm92RteuApZgxYk2pMe-mYvWEBiM=@protonmail.ch>
Date:   Fri, 17 Aug 2018 22:02:31 +0000
From:   ykp@...tonmail.ch
To:     "Theodore Y. Ts'o" <tytso@....edu>
Cc:     "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] Fix gcc-wall warnings about strncpy

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@....edu wrote:

> On Wed, Aug 15, 2018 at 01:46:37PM +0000, ykp@...tonmail.ch wrote:
>
> > From 12109693995386c9941129e65078a4305e72936e Mon Sep 17 00:00:00 2001
> > From: Vladyslav Tsilytskyi ykp@...tonmail.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