[PATCH] fixdep: exit with error code in error branches of do_config_file()

[PATCH] fixdep: exit with error code in error branches of do_config_file()

[Lukas Bulwahn]

do_config_file() should exit with an error code, and not return if it fails
as then the error in do_config_file() would go unnoticed in the current
code and allow the build to continue. The exit with error code will make
the build fail in those very exceptional cases. If this occurs, this
actually indicates a deeper problem in the execution of the kernel build
process.

Now, that the function exists, we do not explicitly free memory and close
the file handlers in do_config_file(), as this is covered by exit().

This issue in the fixdep script was present already before its initial
import into the git repository in 2005 (Linux-2.6.12-rc2). Hence, the Fixes
tag would be imprecise and we do not include a Fixes tag to this commit.

This issue was identified during the review of a previous patch that
intended to address a memory leak detected by a static analysis tool.

Link: https://lkml.org/lkml/2017/12/14/736

Suggested-by: Nicholas Mc Guire <der.herr@hofr.at>
Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
compile tested on top of next-20171220 with clang and gcc

 scripts/basic/fixdep.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index bbf62cb..4274610 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -284,19 +284,18 @@ static void do_config_file(const char *filename)
 		exit(2);
 	}
 	if (st.st_size == 0) {
-		close(fd);
-		return;
+		fprintf(stderr, "fixdep: error empty file config file: ");
+		perror(filename);
+		exit(2);
 	}
 	map = malloc(st.st_size + 1);
 	if (!map) {
 		perror("fixdep: malloc");
-		close(fd);
-		return;
+		exit(2);
 	}
 	if (read(fd, map, st.st_size) != st.st_size) {
 		perror("fixdep: read");
-		close(fd);
-		return;
+		exit(2);
 	}
 	map[st.st_size] = '\0';
 	close(fd);
-- 
2.7.4

Re: [PATCH] fixdep: exit with error code in error branches of do_config_file()

[Nicholas Mc Guire]

On Wed, Dec 20, 2017 at 09:27:02PM +0100, Lukas Bulwahn wrote:
> do_config_file() should exit with an error code, and not return if it fails
> as then the error in do_config_file() would go unnoticed in the current
> code and allow the build to continue. The exit with error code will make
> the build fail in those very exceptional cases. If this occurs, this
> actually indicates a deeper problem in the execution of the kernel build
> process.
> 
> Now, that the function exists, we do not explicitly free memory and close
> the file handlers in do_config_file(), as this is covered by exit().
> 
> This issue in the fixdep script was present already before its initial
> import into the git repository in 2005 (Linux-2.6.12-rc2). Hence, the Fixes
> tag would be imprecise and we do not include a Fixes tag to this commit.
> 

In that case you simply go into the git history tree - thats what it is there
for https://git.kernel.org/cgit/linux/kernel/git/history/history.git/

The problems fixed here were introduced by Kai Germaschewski on
Jun 5 2002 - so

Fixes: commit 04bd72170653 ("kbuild: Make dependencies at compile time")

> This issue was identified during the review of a previous patch that
> intended to address a memory leak detected by a static analysis tool.
> 
> Link: https://lkml.org/lkml/2017/12/14/736
> 
> Suggested-by: Nicholas Mc Guire <der.herr@hofr.at>
> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Reviewed-by: Nicholas Mc Guire <der.herr@hofr.at>

> ---
> compile tested on top of next-20171220 with clang and gcc
> 
>  scripts/basic/fixdep.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> index bbf62cb..4274610 100644
> --- a/scripts/basic/fixdep.c
> +++ b/scripts/basic/fixdep.c
> @@ -284,19 +284,18 @@ static void do_config_file(const char *filename)
>  		exit(2);
>  	}
>  	if (st.st_size == 0) {
> -		close(fd);
> -		return;
> +		fprintf(stderr, "fixdep: error empty file config file: ");
> +		perror(filename);
> +		exit(2);

yup ! the .cmd file should never be empty

>  	}
>  	map = malloc(st.st_size + 1);
>  	if (!map) {
>  		perror("fixdep: malloc");
> -		close(fd);
> -		return;
> +		exit(2);
>  	}
>  	if (read(fd, map, st.st_size) != st.st_size) {
>  		perror("fixdep: read");
> -		close(fd);
> -		return;
> +		exit(2);
>  	}
>  	map[st.st_size] = '\0';
>  	close(fd);
> -- 
> 2.7.4
> 

[PATCH v2] fixdep: exit with error code in error branches of do_config_file()

[Lukas Bulwahn]

do_config_file() should exit with an error code, and not return if it fails
as then the error in do_config_file() would go unnoticed in the current
code and allow the build to continue. The exit with error code will make
the build fail in those very exceptional cases. If this occurs, this
actually indicates a deeper problem in the execution of the kernel build
process.

Now, that the function exists, we do not explicitly free memory and close
the file handlers in do_config_file(), as this is covered by exit().

This issue in the fixdep script was introduced with its initial
implementation back in 2002 by the original author Kai Germaschewski with
this commit 04bd72170653 ("kbuild: Make dependencies at compile time").

This issue was identified during the review of a previous patch that
intended to address a memory leak detected by a static analysis tool.

Link: https://lkml.org/lkml/2017/12/14/736

Fixes: 04bd72170653 ("kbuild: Make dependencies at compile time")
Suggested-by: Nicholas Mc Guire <der.herr@hofr.at>
Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
compile tested on top of next-20171220 with clang and gcc
Change in v2:
  - no code change; only include proper Fixes tag and explain it

 scripts/basic/fixdep.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index bbf62cb..4274610 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -284,19 +284,18 @@ static void do_config_file(const char *filename)
 		exit(2);
 	}
 	if (st.st_size == 0) {
-		close(fd);
-		return;
+		fprintf(stderr, "fixdep: error empty file config file: ");
+		perror(filename);
+		exit(2);
 	}
 	map = malloc(st.st_size + 1);
 	if (!map) {
 		perror("fixdep: malloc");
-		close(fd);
-		return;
+		exit(2);
 	}
 	if (read(fd, map, st.st_size) != st.st_size) {
 		perror("fixdep: read");
-		close(fd);
-		return;
+		exit(2);
 	}
 	map[st.st_size] = '\0';
 	close(fd);
-- 
2.7.4

Re: [PATCH v2] fixdep: exit with error code in error branches of do_config_file()

[Masahiro Yamada]

2017-12-22 4:10 GMT+09:00 Lukas Bulwahn <lukas.bulwahn@gmail.com>:
> do_config_file() should exit with an error code, and not return if it fails
> as then the error in do_config_file() would go unnoticed in the current
> code and allow the build to continue. The exit with error code will make
> the build fail in those very exceptional cases. If this occurs, this
> actually indicates a deeper problem in the execution of the kernel build
> process.
>
> Now, that the function exists, we do not explicitly free memory and close
> the file handlers in do_config_file(), as this is covered by exit().
>
> This issue in the fixdep script was introduced with its initial
> implementation back in 2002 by the original author Kai Germaschewski with
> this commit 04bd72170653 ("kbuild: Make dependencies at compile time").

"04bd72170653" is just confusing.

If you really want to mention this hash,
please clearly say it is in the history repository
outside of this.


> This issue was identified during the review of a previous patch that
> intended to address a memory leak detected by a static analysis tool.
>
> Link: https://lkml.org/lkml/2017/12/14/736
>
> Fixes: 04bd72170653 ("kbuild: Make dependencies at compile time")

Please drop this pointless Fixes tag
because that commit is not reachable from this patch.



> Suggested-by: Nicholas Mc Guire <der.herr@hofr.at>
> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> ---
> compile tested on top of next-20171220 with clang and gcc
> Change in v2:
>   - no code change; only include proper Fixes tag and explain it
>
>  scripts/basic/fixdep.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> index bbf62cb..4274610 100644
> --- a/scripts/basic/fixdep.c
> +++ b/scripts/basic/fixdep.c
> @@ -284,19 +284,18 @@ static void do_config_file(const char *filename)
>                 exit(2);
>         }
>         if (st.st_size == 0) {
> -               close(fd);
> -               return;
> +               fprintf(stderr, "fixdep: error empty file config file: ");
> +               perror(filename);
> +               exit(2);
>         }

No.  This is correct as-is.

do_config_file() does not parse .cmd files
but parse source files (.c .h .S etc.)

Having an empty source file is rare, but possible.






>         map = malloc(st.st_size + 1);
>         if (!map) {
>                 perror("fixdep: malloc");
> -               close(fd);
> -               return;
> +               exit(2);
>         }
>         if (read(fd, map, st.st_size) != st.st_size) {
>                 perror("fixdep: read");
> -               close(fd);
> -               return;
> +               exit(2);
>         }
>         map[st.st_size] = '\0';
>         close(fd);
> --
> 2.7.4
>

These two changes are OK.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] fixdep: exit with error code in error branches of do_config_file()

[Nicholas Mc Guire]

On Sun, Dec 31, 2017 at 01:51:33AM +0900, Masahiro Yamada wrote:
> 2017-12-22 4:10 GMT+09:00 Lukas Bulwahn <lukas.bulwahn@gmail.com>:
> > do_config_file() should exit with an error code, and not return if it fails
> > as then the error in do_config_file() would go unnoticed in the current
> > code and allow the build to continue. The exit with error code will make
> > the build fail in those very exceptional cases. If this occurs, this
> > actually indicates a deeper problem in the execution of the kernel build
> > process.
> >
> > Now, that the function exists, we do not explicitly free memory and close
> > the file handlers in do_config_file(), as this is covered by exit().
> >
> > This issue in the fixdep script was introduced with its initial
> > implementation back in 2002 by the original author Kai Germaschewski with
> > this commit 04bd72170653 ("kbuild: Make dependencies at compile time").
> 
> "04bd72170653" is just confusing.
> 
> If you really want to mention this hash,
> please clearly say it is in the history repository
> outside of this.
> 
> 
> > This issue was identified during the review of a previous patch that
> > intended to address a memory leak detected by a static analysis tool.
> >
> > Link: https://lkml.org/lkml/2017/12/14/736
> >
> > Fixes: 04bd72170653 ("kbuild: Make dependencies at compile time")
> 
> Please drop this pointless Fixes tag
> because that commit is not reachable from this patch.
> 
> 
> 
> > Suggested-by: Nicholas Mc Guire <der.herr@hofr.at>
> > Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > ---
> > compile tested on top of next-20171220 with clang and gcc
> > Change in v2:
> >   - no code change; only include proper Fixes tag and explain it
> >
> >  scripts/basic/fixdep.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> > index bbf62cb..4274610 100644
> > --- a/scripts/basic/fixdep.c
> > +++ b/scripts/basic/fixdep.c
> > @@ -284,19 +284,18 @@ static void do_config_file(const char *filename)
> >                 exit(2);
> >         }
> >         if (st.st_size == 0) {
> > -               close(fd);
> > -               return;
> > +               fprintf(stderr, "fixdep: error empty file config file: ");
> > +               perror(filename);
> > +               exit(2);
> >         }
> 
> No.  This is correct as-is.
> 
> do_config_file() does not parse .cmd files
> but parse source files (.c .h .S etc.)
> 

What case for a legitimately empty source file would there be ?
the files being checked are assumed to be mandatory for the
given configuration right ? if so how can a configuration depend
on an empty file ? would that not point to a config problem and
rather be fixed there ? or am I misunderstanding this here ?

> Having an empty source file is rare, but possible.
>

putting a printf in the empty case handling and running a set of
randconfigs did not reveal such an event (10 runs). 

thx!
hofrat
 
> 
> 
> 
> 
> 
> >         map = malloc(st.st_size + 1);
> >         if (!map) {
> >                 perror("fixdep: malloc");
> > -               close(fd);
> > -               return;
> > +               exit(2);
> >         }
> >         if (read(fd, map, st.st_size) != st.st_size) {
> >                 perror("fixdep: read");
> > -               close(fd);
> > -               return;
> > +               exit(2);
> >         }
> >         map[st.st_size] = '\0';
> >         close(fd);
> > --
> > 2.7.4
> >
> 
> These two changes are OK.
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada

Re: [PATCH v2] fixdep: exit with error code in error branches of do_config_file()

[Masahiro Yamada]

2018-01-01 0:45 GMT+09:00 Nicholas Mc Guire <der.herr@hofr.at>:
> On Sun, Dec 31, 2017 at 01:51:33AM +0900, Masahiro Yamada wrote:
>> 2017-12-22 4:10 GMT+09:00 Lukas Bulwahn <lukas.bulwahn@gmail.com>:
>> > do_config_file() should exit with an error code, and not return if it fails
>> > as then the error in do_config_file() would go unnoticed in the current
>> > code and allow the build to continue. The exit with error code will make
>> > the build fail in those very exceptional cases. If this occurs, this
>> > actually indicates a deeper problem in the execution of the kernel build
>> > process.
>> >
>> > Now, that the function exists, we do not explicitly free memory and close
>> > the file handlers in do_config_file(), as this is covered by exit().
>> >
>> > This issue in the fixdep script was introduced with its initial
>> > implementation back in 2002 by the original author Kai Germaschewski with
>> > this commit 04bd72170653 ("kbuild: Make dependencies at compile time").
>>
>> "04bd72170653" is just confusing.
>>
>> If you really want to mention this hash,
>> please clearly say it is in the history repository
>> outside of this.
>>
>>
>> > This issue was identified during the review of a previous patch that
>> > intended to address a memory leak detected by a static analysis tool.
>> >
>> > Link: https://lkml.org/lkml/2017/12/14/736
>> >
>> > Fixes: 04bd72170653 ("kbuild: Make dependencies at compile time")
>>
>> Please drop this pointless Fixes tag
>> because that commit is not reachable from this patch.
>>
>>
>>
>> > Suggested-by: Nicholas Mc Guire <der.herr@hofr.at>
>> > Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
>> > ---
>> > compile tested on top of next-20171220 with clang and gcc
>> > Change in v2:
>> >   - no code change; only include proper Fixes tag and explain it
>> >
>> >  scripts/basic/fixdep.c | 11 +++++------
>> >  1 file changed, 5 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
>> > index bbf62cb..4274610 100644
>> > --- a/scripts/basic/fixdep.c
>> > +++ b/scripts/basic/fixdep.c
>> > @@ -284,19 +284,18 @@ static void do_config_file(const char *filename)
>> >                 exit(2);
>> >         }
>> >         if (st.st_size == 0) {
>> > -               close(fd);
>> > -               return;
>> > +               fprintf(stderr, "fixdep: error empty file config file: ");
>> > +               perror(filename);
>> > +               exit(2);
>> >         }
>>
>> No.  This is correct as-is.
>>
>> do_config_file() does not parse .cmd files
>> but parse source files (.c .h .S etc.)
>>
>
> What case for a legitimately empty source file would there be ?
> the files being checked are assumed to be mandatory for the
> given configuration right ? if so how can a configuration depend
> on an empty file ? would that not point to a config problem and
> rather be fixed there ? or am I misunderstanding this here ?
>
>> Having an empty source file is rare, but possible.
>>
>
> putting a printf in the empty case handling and running a set of
> randconfigs did not reveal such an event (10 runs).


Remove the comment line in
scripts/mod/empty.c







> thx!
> hofrat
>
>>
>>
>>
>>
>>
>> >         map = malloc(st.st_size + 1);
>> >         if (!map) {
>> >                 perror("fixdep: malloc");
>> > -               close(fd);
>> > -               return;
>> > +               exit(2);
>> >         }
>> >         if (read(fd, map, st.st_size) != st.st_size) {
>> >                 perror("fixdep: read");
>> > -               close(fd);
>> > -               return;
>> > +               exit(2);
>> >         }
>> >         map[st.st_size] = '\0';
>> >         close(fd);
>> > --
>> > 2.7.4
>> >
>>
>> These two changes are OK.
>>
>>
>>
>> --
>> Best Regards
>> Masahiro Yamada
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] fixdep: exit with error code in error branches of do_config_file()

[Nicholas Mc Guire]

On Mon, Jan 01, 2018 at 03:41:10PM +0900, Masahiro Yamada wrote:
> 2018-01-01 0:45 GMT+09:00 Nicholas Mc Guire <der.herr@hofr.at>:
> > On Sun, Dec 31, 2017 at 01:51:33AM +0900, Masahiro Yamada wrote:
> >> 2017-12-22 4:10 GMT+09:00 Lukas Bulwahn <lukas.bulwahn@gmail.com>:
> >> > do_config_file() should exit with an error code, and not return if it fails
> >> > as then the error in do_config_file() would go unnoticed in the current
> >> > code and allow the build to continue. The exit with error code will make
> >> > the build fail in those very exceptional cases. If this occurs, this
> >> > actually indicates a deeper problem in the execution of the kernel build
> >> > process.
> >> >
> >> > Now, that the function exists, we do not explicitly free memory and close
> >> > the file handlers in do_config_file(), as this is covered by exit().
> >> >
> >> > This issue in the fixdep script was introduced with its initial
> >> > implementation back in 2002 by the original author Kai Germaschewski with
> >> > this commit 04bd72170653 ("kbuild: Make dependencies at compile time").
> >>
> >> "04bd72170653" is just confusing.
> >>
> >> If you really want to mention this hash,
> >> please clearly say it is in the history repository
> >> outside of this.
> >>
> >>
> >> > This issue was identified during the review of a previous patch that
> >> > intended to address a memory leak detected by a static analysis tool.
> >> >
> >> > Link: https://lkml.org/lkml/2017/12/14/736
> >> >
> >> > Fixes: 04bd72170653 ("kbuild: Make dependencies at compile time")
> >>
> >> Please drop this pointless Fixes tag
> >> because that commit is not reachable from this patch.
> >>
> >>
> >>
> >> > Suggested-by: Nicholas Mc Guire <der.herr@hofr.at>
> >> > Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> >> > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> >> > ---
> >> > compile tested on top of next-20171220 with clang and gcc
> >> > Change in v2:
> >> >   - no code change; only include proper Fixes tag and explain it
> >> >
> >> >  scripts/basic/fixdep.c | 11 +++++------
> >> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> >> > index bbf62cb..4274610 100644
> >> > --- a/scripts/basic/fixdep.c
> >> > +++ b/scripts/basic/fixdep.c
> >> > @@ -284,19 +284,18 @@ static void do_config_file(const char *filename)
> >> >                 exit(2);
> >> >         }
> >> >         if (st.st_size == 0) {
> >> > -               close(fd);
> >> > -               return;
> >> > +               fprintf(stderr, "fixdep: error empty file config file: ");
> >> > +               perror(filename);
> >> > +               exit(2);
> >> >         }
> >>
> >> No.  This is correct as-is.
> >>
> >> do_config_file() does not parse .cmd files
> >> but parse source files (.c .h .S etc.)
> >>
> >
> > What case for a legitimately empty source file would there be ?
> > the files being checked are assumed to be mandatory for the
> > given configuration right ? if so how can a configuration depend
> > on an empty file ? would that not point to a config problem and
> > rather be fixed there ? or am I misunderstanding this here ?
> >
> >> Having an empty source file is rare, but possible.
> >>
> >
> > putting a printf in the empty case handling and running a set of
> > randconfigs did not reveal such an event (10 runs).
> 
> 
> Remove the comment line in
> scripts/mod/empty.c
> 
>
And would that file be acceptable if it were actually empty

The point is what would be the rational for a file that
is mandatory but empty - my assumption is that this file would not
be acceptable if it were actually empty simply because it would
be impossible to figure out what it is good for and readability/documentation
is a key, if not the key, issue in a softare element the complexity
of the linux kernel.

The argument for the exit(2) here would be that an empty file 
would indicate that something is going wrong with high probablity
and jumping that event could result in broken builds.

thx!
hofrat
 
> 
> > thx!
> > hofrat
> >
> >>
> >>
> >>
> >>
> >>
> >> >         map = malloc(st.st_size + 1);
> >> >         if (!map) {
> >> >                 perror("fixdep: malloc");
> >> > -               close(fd);
> >> > -               return;
> >> > +               exit(2);
> >> >         }
> >> >         if (read(fd, map, st.st_size) != st.st_size) {
> >> >                 perror("fixdep: read");
> >> > -               close(fd);
> >> > -               return;
> >> > +               exit(2);
> >> >         }
> >> >         map[st.st_size] = '\0';
> >> >         close(fd);
> >> > --
> >> > 2.7.4
> >> >
> >>
> >> These two changes are OK.
> >>
> >>
> >>
> >> --
> >> Best Regards
> >> Masahiro Yamada
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada

Re: [PATCH v2] fixdep: exit with error code in error branches of do_config_file()

[Sam Ravnborg]

> >
> And would that file be acceptable if it were actually empty

There have been cases with empty files in the source tree.
And it is not the job of fixdep to do anything special about them.
fixdep shall do what it is requested to do, and not more than
that.

	Sam

[PATCH v3] fixdep: exit with error code in error branches of do_config_file()

[Lukas Bulwahn]

do_config_file() should exit with an error code on internal run-time
errors, and not return if it fails as then the error in do_config_file()
would go unnoticed in the current code and allow the build to continue.
The exit with error code will make the build fail in those very
exceptional cases. If this occurs, this actually indicates a deeper
problem in the execution of the kernel build process.

Now, in these error cases, we do not explicitly free memory and close
the file handlers in do_config_file(), as this is covered by exit().

This issue in the fixdep script was introduced with its initial
implementation back in 2002 by the original author Kai Germaschewski with
this commit 04bd72170653 ("kbuild: Make dependencies at compile time")
in the linux history git tree, i.e.,
git://git.kernel.org/pub/scm/linux/kernel/git/history/history.git.

This issue was identified during the review of a previous patch that
intended to address a memory leak detected by a static analysis tool.

Link: https://lkml.org/lkml/2017/12/14/736

Suggested-by: Nicholas Mc Guire <der.herr@hofr.at>
Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
compile tested on top of next-20180108 with clang and gcc
Changes in v2:
  - no code change; only include proper Fixes tag and explain it
Changes in v3:
  - Clarify history commit reference and dropped Fixes tag
  - Do not error on empty files (reverts one hunk of v2)

 scripts/basic/fixdep.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index bbf62cb..86a61d6 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -290,13 +290,11 @@ static void do_config_file(const char *filename)
 	map = malloc(st.st_size + 1);
 	if (!map) {
 		perror("fixdep: malloc");
-		close(fd);
-		return;
+		exit(2);
 	}
 	if (read(fd, map, st.st_size) != st.st_size) {
 		perror("fixdep: read");
-		close(fd);
-		return;
+		exit(2);
 	}
 	map[st.st_size] = '\0';
 	close(fd);
-- 
2.5.5

Re: [PATCH v2] fixdep: exit with error code in error branches of do_config_file()

[Lukas Bulwahn]

On Sun, 31 Dec 2017, Masahiro Yamada wrote:

> 2017-12-22 4:10 GMT+09:00 Lukas Bulwahn <lukas.bulwahn@gmail.com>:
>> diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
>> index bbf62cb..4274610 100644
>> --- a/scripts/basic/fixdep.c
>> +++ b/scripts/basic/fixdep.c
>> @@ -284,19 +284,18 @@ static void do_config_file(const char *filename)
>>                 exit(2);
>>         }
>>         if (st.st_size == 0) {
>> -               close(fd);
>> -               return;
>> +               fprintf(stderr, "fixdep: error empty file config file: ");
>> +               perror(filename);
>> +               exit(2);
>>         }
>
> No.  This is correct as-is.
>
> do_config_file() does not parse .cmd files
> but parse source files (.c .h .S etc.)
>
> Having an empty source file is rare, but possible.
>
>

Now that I looked at the code for creating the v3 patch, I am confused 
about the error messages in scripts/basic/fixdep.c, lines 275 - 285, in 
do_config_file():

         fd = open(filename, O_RDONLY);
         if (fd < 0) {
                 fprintf(stderr, "fixdep: error opening config file: ");
                 perror(filename);
                 exit(2);
         }
         if (fstat(fd, &st) < 0) {
                 fprintf(stderr, "fixdep: error fstat'ing config file: ");
                 perror(filename);
                 exit(2);
         }

These error messages suggest that filename (and the file handler fd) 
refers to a config file, but you say that filename and fd refer to source 
files.

Looking at parse_dep_file() and how it invokes do_config_file(), I think 
you are right that it does refer to source files.

If you confirm that this is correct, I would change `config file` to 
`source file` in the error messages of do_config_file().

What is best to do here, provide a separate patch or add it to the 
existing patch?

Lukas

Re: [PATCH v3] fixdep: exit with error code in error branches of do_config_file()

[Masahiro Yamada]

2018-01-08 19:04 GMT+09:00 Lukas Bulwahn <lukas.bulwahn@gmail.com>:
> do_config_file() should exit with an error code on internal run-time
> errors, and not return if it fails as then the error in do_config_file()
> would go unnoticed in the current code and allow the build to continue.
> The exit with error code will make the build fail in those very
> exceptional cases. If this occurs, this actually indicates a deeper
> problem in the execution of the kernel build process.
>
> Now, in these error cases, we do not explicitly free memory and close
> the file handlers in do_config_file(), as this is covered by exit().
>
> This issue in the fixdep script was introduced with its initial
> implementation back in 2002 by the original author Kai Germaschewski with
> this commit 04bd72170653 ("kbuild: Make dependencies at compile time")
> in the linux history git tree, i.e.,
> git://git.kernel.org/pub/scm/linux/kernel/git/history/history.git.
>
> This issue was identified during the review of a previous patch that
> intended to address a memory leak detected by a static analysis tool.
>
> Link: https://lkml.org/lkml/2017/12/14/736
>
> Suggested-by: Nicholas Mc Guire <der.herr@hofr.at>
> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> ---
> compile tested on top of next-20180108 with clang and gcc
> Changes in v2:
>   - no code change; only include proper Fixes tag and explain it
> Changes in v3:
>   - Clarify history commit reference and dropped Fixes tag
>   - Do not error on empty files (reverts one hunk of v2)
>
>  scripts/basic/fixdep.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> index bbf62cb..86a61d6 100644
> --- a/scripts/basic/fixdep.c
> +++ b/scripts/basic/fixdep.c
> @@ -290,13 +290,11 @@ static void do_config_file(const char *filename)
>         map = malloc(st.st_size + 1);
>         if (!map) {
>                 perror("fixdep: malloc");
> -               close(fd);
> -               return;
> +               exit(2);
>         }
>         if (read(fd, map, st.st_size) != st.st_size) {
>                 perror("fixdep: read");
> -               close(fd);
> -               return;
> +               exit(2);
>         }
>         map[st.st_size] = '\0';
>         close(fd);
> --
> 2.5.5
>

Applied to linux-kbuild/kbuid. Thanks!



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] fixdep: exit with error code in error branches of do_config_file()

[Masahiro Yamada]

2018-01-08 19:17 GMT+09:00 Lukas Bulwahn <lukas.bulwahn@gmail.com>:
>
> On Sun, 31 Dec 2017, Masahiro Yamada wrote:
>
>> 2017-12-22 4:10 GMT+09:00 Lukas Bulwahn <lukas.bulwahn@gmail.com>:
>>>
>>> diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
>>> index bbf62cb..4274610 100644
>>> --- a/scripts/basic/fixdep.c
>>> +++ b/scripts/basic/fixdep.c
>>> @@ -284,19 +284,18 @@ static void do_config_file(const char *filename)
>>>                 exit(2);
>>>         }
>>>         if (st.st_size == 0) {
>>> -               close(fd);
>>> -               return;
>>> +               fprintf(stderr, "fixdep: error empty file config file:
>>> ");
>>> +               perror(filename);
>>> +               exit(2);
>>>         }
>>
>>
>> No.  This is correct as-is.
>>
>> do_config_file() does not parse .cmd files
>> but parse source files (.c .h .S etc.)
>>
>> Having an empty source file is rare, but possible.
>>
>>
>
> Now that I looked at the code for creating the v3 patch, I am confused about
> the error messages in scripts/basic/fixdep.c, lines 275 - 285, in
> do_config_file():


Not only the error messages.

Function names are confusing too.

parse_config_file(), do_config_file
  these are not for parsing configuration file,
  but for parsing files that contain CONFIG options



>
>         fd = open(filename, O_RDONLY);
>         if (fd < 0) {
>                 fprintf(stderr, "fixdep: error opening config file: ");
>                 perror(filename);
>                 exit(2);
>         }
>         if (fstat(fd, &st) < 0) {
>                 fprintf(stderr, "fixdep: error fstat'ing config file: ");
>                 perror(filename);
>                 exit(2);
>         }
>
> These error messages suggest that filename (and the file handler fd) refers
> to a config file, but you say that filename and fd refer to source files.
>
> Looking at parse_dep_file() and how it invokes do_config_file(), I think you
> are right that it does refer to source files.

This depends on the definition of "source file".

In general, we think "source files" refer to files processed by compiler,
like *c, *.h, *.S, *.dts, etc.

In the fixdep context, "source file" has narrower meaning.

You will notice this from the comments in parse_dep_file(),
"Do not list the source file as dependency, ..."


> If you confirm that this is correct, I would change `config file` to `source
> file` in the error messages of do_config_file().

Maybe a bad idea.
For the reason above, "source file" is also confusing in my opinion.

Just "file" should be fine.  But I do not need a patch just for
removing "config".


I'd like to refactor the code in a bigger picture.
do_config_file() and print_deps() can be merged together,
since they do similar things.
After merged, load_file() will refer "file".

https://patchwork.kernel.org/patch/10151165/
https://patchwork.kernel.org/patch/10151157/

I had already had those cleanups in my mind
but I was holding it back to avoid conflicts with your patch.



> What is best to do here, provide a separate patch or add it to the existing
> patch?
>
> Lukas



-- 
Best Regards
Masahiro Yamada