Re: [PATCH v2] fixdep: check return value of printf() and putchar()

[Masahiro Yamada]

On Tue, Jun 25, 2019 at 3:54 PM Masahiro Yamada
<yamada.masahiro@xxxxxxxxxxxxx> wrote:
>
> When there is not enough space on your storage device, the build will
> fail with 'No space left on device' error message.
>
> The reason is obvious from the message, so you will free up some disk
> space, then you will resume the build.
>
> However, sometimes you may still see a mysterious error message:
>
>   unterminated call to function 'wildcard': missing ')'.
>
> If you run out of the disk space, fixdep may end up with generating
> incomplete .*.cmd files.
>
> For example, if the disk shortage occurs while fixdep is running
> print_dep(), the .*.cmd might be truncated like this:
>
>    $(wildcard include/config/
>
> When you run 'make' next time, this broken .*.cmd will be included,
> then GNU Make will terminate parsing since it is a wrong syntax.
>
> Once this happens, you need to run 'make clean' or delete the broken
> .*.cmd file manually.
>
> Even if you do not see any error message, the .*.cmd files after any
> error could be potentially incomplete, and unreliable. You may miss
> the re-compilation due to missing header dependency.
>
> If printf() cannot output the string for disk shortage or whatever
> reason, it returns a negative value, but currently fixdep does not
> check it at all. Consequently, fixdep *successfully* generates a
> broken .*.cmd file. Make never notices that since fixdep exits with 0,
> which means success.
>
> Given the intended usage of fixdep, it must respect the return value
> of not only malloc(), but also printf() and putchar().
>
> This seems a long-standing issue since the introduction of fixdep.
>
> In old days, Kbuild tried to provide an extra safety by letting fixdep
> output to a temporary file and renaming it after everything is done:
>
>   scripts/basic/fixdep $(depfile) $@ '$(make-cmd)' > $(dot-target).tmp;\
>   rm -f $(depfile);                                                    \
>   mv -f $(dot-target).tmp $(dot-target).cmd)
>
> It did not avoid the current issue; fixdep created a truncated tmp file
> reporting success, so the broken tmp would be renamed to a .*.cmd file.
>
> By propagating the error code to the build system, this problem should
> be solved because:
>
> [1] Since commit 9c2af1c7377a ("kbuild: add .DELETE_ON_ERROR special
>     target"), Make will delete the target automatically on any failure
>     in the recipe.
>
> [2] Since commit 392885ee82d3 ("kbuild: let fixdep directly write to
>     .*.cmd files"), .*.cmd file is included only when the corresponding
>     target already exists.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
> ---

Applied to linux-kbuild.


> Changes in v2:
>  - Add prror()
>
>  scripts/basic/fixdep.c | 51 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 41 insertions(+), 10 deletions(-)
>
> diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> index facbd603adf6..4ac973f2dc8c 100644
> --- a/scripts/basic/fixdep.c
> +++ b/scripts/basic/fixdep.c
> @@ -99,6 +99,7 @@
>  #include <unistd.h>
>  #include <fcntl.h>
>  #include <string.h>
> +#include <stdarg.h>
>  #include <stdlib.h>
>  #include <stdio.h>
>  #include <ctype.h>
> @@ -109,6 +110,36 @@ static void usage(void)
>         exit(1);
>  }
>
> +/*
> + * In the intended usage of this program, the stdout is redirected to .*.cmd
> + * The return value of printf() and putchar() must be checked to catch any
> + * error like "No space left on device".
> + */
> +static void xprintf(const char *format, ...)
> +{
> +       va_list ap;
> +       int ret;
> +
> +       va_start(ap, format);
> +       ret = vprintf(format, ap);
> +       if (ret < 0) {
> +               perror("fixdep");
> +               exit(1);
> +       }
> +       va_end(ap);
> +}
> +
> +static void xputchar(int c)
> +{
> +       int ret;
> +
> +       ret = putchar(c);
> +       if (ret == EOF) {
> +               perror("fixdep");
> +               exit(1);
> +       }
> +}
> +
>  /*
>   * Print out a dependency path from a symbol name
>   */
> @@ -116,7 +147,7 @@ static void print_dep(const char *m, int slen, const char *dir)
>  {
>         int c, prev_c = '/', i;
>
> -       printf("    $(wildcard %s/", dir);
> +       xprintf("    $(wildcard %s/", dir);
>         for (i = 0; i < slen; i++) {
>                 c = m[i];
>                 if (c == '_')
> @@ -124,10 +155,10 @@ static void print_dep(const char *m, int slen, const char *dir)
>                 else
>                         c = tolower(c);
>                 if (c != '/' || prev_c != '/')
> -                       putchar(c);
> +                       xputchar(c);
>                 prev_c = c;
>         }
> -       printf(".h) \\\n");
> +       xprintf(".h) \\\n");
>  }
>
>  struct item {
> @@ -324,13 +355,13 @@ static void parse_dep_file(char *m, const char *target)
>                                  */
>                                 if (!saw_any_target) {
>                                         saw_any_target = 1;
> -                                       printf("source_%s := %s\n\n",
> -                                              target, m);
> -                                       printf("deps_%s := \\\n", target);
> +                                       xprintf("source_%s := %s\n\n",
> +                                               target, m);
> +                                       xprintf("deps_%s := \\\n", target);
>                                 }
>                                 is_first_dep = 0;
>                         } else {
> -                               printf("  %s \\\n", m);
> +                               xprintf("  %s \\\n", m);
>                         }
>
>                         buf = read_file(m);
> @@ -353,8 +384,8 @@ static void parse_dep_file(char *m, const char *target)
>                 exit(1);
>         }
>
> -       printf("\n%s: $(deps_%s)\n\n", target, target);
> -       printf("$(deps_%s):\n", target);
> +       xprintf("\n%s: $(deps_%s)\n\n", target, target);
> +       xprintf("$(deps_%s):\n", target);
>  }
>
>  int main(int argc, char *argv[])
> @@ -369,7 +400,7 @@ int main(int argc, char *argv[])
>         target = argv[2];
>         cmdline = argv[3];
>
> -       printf("cmd_%s := %s\n\n", target, cmdline);
> +       xprintf("cmd_%s := %s\n\n", target, cmdline);
>
>         buf = read_file(depfile);
>         parse_dep_file(buf, target);
> --
> 2.17.1
>


-- 
Best Regards
Masahiro Yamada