linux-ext4 - Re: [PATCH] ext2/e2fsprogs: fix cppcheck warnings

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 13 Aug 2018 08:50:52 +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] ext2/e2fsprogs: fix cppcheck warnings

> So.... why?

There is no great reason behind. I believe that evidently buggy code
needs to be fixed (or removed).
Yeah, cppcheck is not the best tool. In this case it was a way for me
to get along with both: e2fsprogs (I need a starting point to explore
the code) and cppcheck. I'm not going to run this static analysis
tool on a regular basis, treat it as a learning step.

Regards, Vlad

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On August 13, 2018 2:23 AM, Theodore Y. Ts'o tytso@....edu wrote:

> On Sun, Aug 12, 2018 at 07:47:15PM +0000, ykp@...tonmail.ch wrote:
>
> > From bbcbf9936c739e53327a9bee4790a167f34fc04a Mon Sep 17 00:00:00 2001
> > From: Vladyslav Tsilytskyi ykp@...tonmail.ch
> > Date: Sun, 12 Aug 2018 21:34:42 +0200
> > Subject: [PATCH] Fix static analysis warnings.
> > Prevented file descriptor leaks in localealias.c
> > "!length" code in tdb.c didn't make any sense because variable was
> > uninitialized.
> > Swapped blocks in dosio.c to prevent "part" variable leak if any
> > return from previous block executed.
>
> None of these changes are actually compiled on Linux. In fact,
> dosio.c is never compiled; it's not even mentioned in any of the
> Makefiles. The dosio.c file is one that I'm actually quite tempted to
> just delete, since it's likely nt_io.c is more likely to be usable.
> I'm also extremely tempted to just delete all of the intl directory.
> That directory is never compiled for Linux, since the libintl library
> is provided by glibc. And it's badly out of date. It's from
> gettext-0.14.1, and the latest version of gettext is 0.19.1. The
> problem is integrating the intl sources into the e2fsprogs build
> system. It's not hard work per se, but it's finicky, and it's only
> useful if you want to build e2fsprogs with I18N support on say,
> Solaris, AIX, NetBSD, etc. And I just don't care that much. The more
> we diverge from the intl sources from the GNU gettext page, the harder
> it's going to sync up with newer versions.
> Fixing these sorts of problems really isn't worth it. That's because
> I really don't like to apply super-complex patches without very
> careful desk checking, especially if I can't test the changes easily.
> And these are just resource leaks on error cases, and none of these
> are likely to be serious leaks (e.g., they are one-off leaks, and not
> something that would be continually leaking and causing long-running
> programs to eventually run out of memory). The argument of silencing
> warnings to make it easier to spot "real" bugs is also not very
> compelling for cppcheck, because the level of checking it does is very
> low quality. We can do much better using GCC -Wall, Clang warnings,
> and Coverity. So.... why?
> The tdb patch does fix a real bug, although it's completely irrelevant
> for Linux (since we have strdup, some we don't need the replacement).
> Fortunately it's a simple enough patch that it's obviously correct
> from inspection. This is why I told you privately this was the only
> fix that I considered worth it.
> Cheers,
>
> -   Ted