ext2/e2fsprogs: fix cppcheck warnings

From bbcbf9936c739e53327a9bee4790a167f34fc04a Mon Sep 17 00:00:00 2001
From: Vladyslav Tsilytskyi <ykp@protonmail.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.

Signed-off-by: Vladyslav Tsilytskyi <ykp@protonmail.ch>
---
 intl/localealias.c | 10 ++++++++--
 lib/ext2fs/dosio.c |  9 ++++++---
 lib/ext2fs/tdb.c   |  3 +--
 3 files changed, 15 insertions(+), 7 deletions(-)

--
2.17.1

On Sun, Aug 12, 2018 at 07:47:15PM +0000, ykp@protonmail.ch wrote:
> From bbcbf9936c739e53327a9bee4790a167f34fc04a Mon Sep 17 00:00:00 2001
> From: Vladyslav Tsilytskyi <ykp@protonmail.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

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

> On Sun, Aug 12, 2018 at 07:47:15PM +0000, ykp@protonmail.ch wrote:
>
> > From bbcbf9936c739e53327a9bee4790a167f34fc04a Mon Sep 17 00:00:00 2001
> > From: Vladyslav Tsilytskyi ykp@protonmail.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

On Mon, Aug 13, 2018 at 08:50:52AM +0000, ykp@protonmail.ch wrote:
> > 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.

If you want to do code cleanup, it's better to either look for Clang
warnings or gcc-wall warnings.  The first can be done via "CC=clang
configure".  The second can be done via running "make gcc-wall" in a
particular build directory.  After you fix gcc-wall issues, you can
run "make gcc-wall-new" to only run gcc -Wall on the modified files.

You can run the test_script in the tests directory with the --valgrind
or --valgrind-leakcheck.

In some cases we've deliberately neelded not fixed a warning when it's
not worth it.  Long-term maintainability and code readability is
important.

One file where a lot of cleanup can be needed --- not just blindly
cleaning up gcc -Wall or clang warnings, but rather restructing and
general code cleanup to make the code cleaner and consistent with
general e2fsprogs code quality and style --- is misc/e4defrag.c.
There is some interest by Jaco to add new featuers on e4defrag, so if
that is something you are interested in doing somme cleanup work on,
we'll need to do some air traffic control to avoid change conflicts.

Is there something specific you are interested in working on?
Finally, one potential issue.  Since you are working under an
encrypted channel and you aren't specifying your name, I assume you
are concerned about preserving your anonymity.  One of the problems is
that if you are making code contributions, I need to know that who you
are.  It doesn't have to be public --- you can let me know in private
--- but I do need to know your identity.  There is precedence for this
--- "George Spellvin" is an occasional contributor to the Linux
kernel, but Linus Torvalds know who he is, and that's been considered
sufficient.  Please see the description of the Developers
Certification of Origin (e.g., the "Signed-off-by" header) for the
background about what it is that we require code contributors to agree
when they contribute code.

Cheers,

						- Ted

Thank you for pointing places where some job can be done.
I'll definitely check it and do my best to clean it up.

> Is there something specific you are interested in working on?

My original goal was to find out how fsck works, check if there
is a standard API for all set of fsck.* utilities and as a result
write a new one for fuse.cryfs filesystem.

Regards,
Vlad

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On August 13, 2018 4:37 PM, Theodore Y. Ts'o <tytso@mit.edu> wrote:

> On Mon, Aug 13, 2018 at 08:50:52AM +0000, ykp@protonmail.ch wrote:
>
> > > 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.
>
> If you want to do code cleanup, it's better to either look for Clang
> warnings or gcc-wall warnings. The first can be done via "CC=clang
> configure". The second can be done via running "make gcc-wall" in a
> particular build directory. After you fix gcc-wall issues, you can
> run "make gcc-wall-new" to only run gcc -Wall on the modified files.
>
> You can run the test_script in the tests directory with the --valgrind
> or --valgrind-leakcheck.
>
> In some cases we've deliberately neelded not fixed a warning when it's
> not worth it. Long-term maintainability and code readability is
> important.
>
> One file where a lot of cleanup can be needed --- not just blindly
> cleaning up gcc -Wall or clang warnings, but rather restructing and
> general code cleanup to make the code cleaner and consistent with
> general e2fsprogs code quality and style --- is misc/e4defrag.c.
> There is some interest by Jaco to add new featuers on e4defrag, so if
> that is something you are interested in doing somme cleanup work on,
> we'll need to do some air traffic control to avoid change conflicts.
>
> Is there something specific you are interested in working on?
> Finally, one potential issue. Since you are working under an
> encrypted channel and you aren't specifying your name, I assume you
> are concerned about preserving your anonymity. One of the problems is
> that if you are making code contributions, I need to know that who you
> are. It doesn't have to be public --- you can let me know in private
> --- but I do need to know your identity. There is precedence for this
> --- "George Spellvin" is an occasional contributor to the Linux
> kernel, but Linus Torvalds know who he is, and that's been considered
> sufficient. Please see the description of the Developers
> Certification of Origin (e.g., the "Signed-off-by" header) for the
> background about what it is that we require code contributors to agree
> when they contribute code.
>
> Cheers,
>
> - Ted

Ted,

gcc-wall generate a lot of noice, for example 45 such warnings in /debugfs/debugfs.c:

> ../../debugfs/debugfs.c:221:49: warning: declaration of ‘sci_idx’ shadows a global declaration [-Wshadow]
> void do_open_filesys(int argc, char **argv, int sci_idx EXT2FS_ATTR((unused)),
> ../../debugfs/debugfs.h:28:12: note: shadowed declaration is here
> extern int sci_idx;
> ^~~~~~~

Of course I can patch all these places like this:

> diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
> index e03519c4..6d0e9cdb 100644
> --- a/debugfs/debugfs.c
> +++ b/debugfs/debugfs.c
> @@ -218,7 +218,7 @@ errout:
>  		com_err(device, retval, "while trying to close filesystem");
>  }
>
> -void do_open_filesys(int argc, char **argv, int sci_idx EXT2FS_ATTR((unused)),
> +void do_open_filesys(int argc, char **argv, int _sci_idx EXT2FS_ATTR((unused)),
>  		     void *infop EXT2FS_ATTR((unused)))
>  {
>  	int	c, err;

But is it worth to?

I've tried clang as you suggested.
I've replaced "CC = gcc" by "CC = clang" or even "CC = clang -Weverything -pedantic",
but output remains the same as if I just normally compile the project with make and gcc.
What is wrong?

Regards,
Vlad

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On August 13, 2018 4:50 PM, ykp@protonmail.ch wrote:

> Thank you for pointing places where some job can be done.
> I'll definitely check it and do my best to clean it up.
>
> > Is there something specific you are interested in working on?
>
> My original goal was to find out how fsck works, check if there
> is a standard API for all set of fsck.* utilities and as a result
> write a new one for fuse.cryfs filesystem.
> Regards,
> Vlad
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On August 13, 2018 4:37 PM, Theodore Y. Ts'o tytso@mit.edu wrote:
>
> > On Mon, Aug 13, 2018 at 08:50:52AM +0000, ykp@protonmail.ch wrote:
> >
> > > > 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.
> >
> > If you want to do code cleanup, it's better to either look for Clang
> > warnings or gcc-wall warnings. The first can be done via "CC=clang
> > configure". The second can be done via running "make gcc-wall" in a
> > particular build directory. After you fix gcc-wall issues, you can
> > run "make gcc-wall-new" to only run gcc -Wall on the modified files.
> > You can run the test_script in the tests directory with the --valgrind
> > or --valgrind-leakcheck.
> > In some cases we've deliberately neelded not fixed a warning when it's
> > not worth it. Long-term maintainability and code readability is
> > important.
> > One file where a lot of cleanup can be needed --- not just blindly
> > cleaning up gcc -Wall or clang warnings, but rather restructing and
> > general code cleanup to make the code cleaner and consistent with
> > general e2fsprogs code quality and style --- is misc/e4defrag.c.
> > There is some interest by Jaco to add new featuers on e4defrag, so if
> > that is something you are interested in doing somme cleanup work on,
> > we'll need to do some air traffic control to avoid change conflicts.
> > Is there something specific you are interested in working on?
> > Finally, one potential issue. Since you are working under an
> > encrypted channel and you aren't specifying your name, I assume you
> > are concerned about preserving your anonymity. One of the problems is
> > that if you are making code contributions, I need to know that who you
> > are. It doesn't have to be public --- you can let me know in private
> > --- but I do need to know your identity. There is precedence for this
> > --- "George Spellvin" is an occasional contributor to the Linux
> > kernel, but Linus Torvalds know who he is, and that's been considered
> > sufficient. Please see the description of the Developers
> > Certification of Origin (e.g., the "Signed-off-by" header) for the
> > background about what it is that we require code contributors to agree
> > when they contribute code.
> > Cheers,
> >
> > -   Ted

On Wed, Aug 15, 2018 at 01:56:42PM +0000, ykp@protonmail.ch wrote:
>
> gcc-wall generate a lot of noice, for example 45 such warnings in /debugfs/debugfs.c:
> 
> > ../../debugfs/debugfs.c:221:49: warning: declaration of ‘sci_idx’ shadows a global declaration [-Wshadow]
> > void do_open_filesys(int argc, char **argv, int sci_idx EXT2FS_ATTR((unused)),
> > ../../debugfs/debugfs.h:28:12: note: shadowed declaration is here
> > extern int sci_idx;
> > ^~~~~~~

Yeah, that's something which I had thought I had fixed, but I forgot a
rename in debugfs.h.  I'll fix it upstream:

diff --git a/debugfs/debugfs.h b/debugfs/debugfs.h
index 449740be8..d1d13b455 100644
--- a/debugfs/debugfs.h
+++ b/debugfs/debugfs.h
@@ -25,7 +25,7 @@
 extern ext2_filsys current_fs;
 extern quota_ctx_t current_qctx;
 extern ext2_ino_t	root, cwd;
-extern int sci_idx;
+extern int ss_sci_idx;
 extern ss_request_table debug_cmds, extent_cmds;
 
 extern void reset_getopt(void);

Yes, even with the above fix, gcc-wall is a bit noise.  I don't always
fix all gcc warnings....

> I've tried clang as you suggested.
> I've replaced "CC = gcc" by "CC = clang" or even "CC = clang -Weverything -pedantic",
> but output remains the same as if I just normally compile the project with make and gcc.
> What is wrong?

Here's an example:

<tytso@cwcc> {/tmp/e2fsprogs}  
1070% CC=clang /usr/projects/e2fsprogs/e2fsprogs/configure
      ...
<tytso@cwcc> {/tmp/e2fsprogs}  
1070% grep clang MCONFIG 
CC = clang
BUILD_CC = clang
LD = $(PURE) clang

You can verify what C compiler and cc flags are in use by using V=1
(this a convention from Kernel builds):

<tytso@cwcc> {/tmp/e2fsprogs}  
1071% cd util ; make V=1
echo "/* fake dirpaths.h for config.h */" > dirpaths.h
clang -c -g -flto -ffat-lto-objects    -g -O2  -I. -I../lib -I/usr/projects/e2fsprogs/e2fsprogs/lib -DHAVE_CONFIG_H /usr/projects/e2fsprogs/e2fsprogs/util/subst.c -o subst.o
clang: warning: optimization flag '-ffat-lto-objects' is not supported [-Wignored-optimization-argument]
clang -g -flto -ffat-lto-objects     -o subst subst.o
clang -c -g -flto -ffat-lto-objects    -g -O2  -I. -I../lib -I/usr/projects/e2fsprogs/e2fsprogs/lib -DHAVE_CONFIG_H /usr/projects/e2fsprogs/e2fsprogs/util/symlinks.c -o symlinks.o
clang: warning: optimization flag '-ffat-lto-objects' is not supported [-Wignored-optimization-argument]
clang -g -flto -ffat-lto-objects     -o symlinks symlinks.o

(The complaint about -ffat-lto-objects is recent, since we only
recently added LTO support --- the next/master branch is our
development branch and so will often be new gcc warnings that pop up.
LTO support is pretty new, and doesn't work the same across gcc/clang
or different versions of gcc for that matter.  I'm going to change the
autoconf file to not enable LTO by default, since it our current LTO
is clearly broken for clang.)

To learn more about the build system, look at MCONFIG.in in the source
directory, MCONFIG in the build directory, and look first dozen lines
or so of e2fsck/Makefile.in, libext2fs/Makefile.in, etc.

						- Ted