Avoid usage of uninit members of ctx

[lucic71]

if (ctx.archive.zip64) { is used uninitialized if ctx.archive.zip64 = zip64; is not executed. Memset'ing the whole struct at the beginning of the function avoids this problem.

[solardiz]

Thank you @lucic71! Can you please adjust the commit message (amend and force-push) so that the title line starts with zip2john: and further lines are wrapped at 75?

In the memset, I'd use sizeof(ctx), or alternatively just add = {} as the initializer, or initialize only the zip64 field (add ctx.archive.zip64 = 0;) because it's the only one used before reaching init_zip_context. But your change is also fine as-is.

@claudioandre-br We're getting CircleCI failures here:

Starting container ghcr.io/claudioandre-br/john-ci:fedora.latest
  image cache not found on this host, downloading ghcr.io/claudioandre-br/john-ci:fedora.latest

  Error pulling image ghcr.io/claudioandre-br/john-ci:fedora.latest: Error response from daemon: Head "https://ghcr.io/v2/claudioandre-br/john-ci/manifests/fedora.latest": denied

[lucic71]

@solardiz done!

As an afterthought, do you think it would be beneficial to have clang-tidy (or other static analysis tool) integrated in the project? I expect that this error would be easily caught by such a tool.

[solardiz]

Thank you! Merged.

As an afterthought, do you think it would be beneficial to have clang-tidy (or other static analysis tool) integrated in the project? I expect that this error would be easily caught by such a tool.

I'm not familiar with clang-tidy in particular. I think this sort of error could also be caught by compiler warnings in general, but compilers' detection of uninitialized accesses varies with optimization levels, etc. I think yes, we could add static analysis to our CI setup, however I think right now we're not even detecting compiler warnings in there - we could by adding -Werror, but I'm afraid there are currently some unfixed ones seen with some compilers, which we'll need to fix first (we have open issues). Maybe you want to help with that? Thank you!

[lucic71]

@solardiz I ran clang-tidy over zip2john.c to see if there is some interesting output. I got:

/home/lucian/benchmarks/john/src/zip2john.c:513:3: error: Value stored to 'Size' is never read [clang-analyzer-deadcode.DeadStores,-warnings-as-errors]
                Size = fget32LE(fp);                                                                     
                ^      ~~~~~~~~~~~~
/home/lucian/benchmarks/john/src/zip2john.c:545:3: error: Value stored to 'Flags' is never read [clang-analyzer-deadcode.DeadStores,-warnings-as-errors]
                Flags = fget16LE(fp);                                                                    
                ^       ~~~~~~~~~~~~     
/home/lucian/benchmarks/john/src/zip2john.c:948:4: error: Call to function 'strcat' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcat'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy,-warnings-as-errors]
                        strcat(filenames, ", ");
                        ^~~~~~
/home/lucian/benchmarks/john/src/zip2john.c:949:4: error: Call to function 'strcat' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcat'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy,-warnings-as-errors]
                        strcat(filenames, ctx->best_files[i].file_name);     
                        ^~~~~~      
/home/lucian/benchmarks/john/src/zip2john.c:1058:6: error: Branch condition evaluates to a garbage value [clang-analyzer-core.uninitialized.Branch,-warnings-as-errors]
        if (ctx.archive.zip64) {

Did you see these errors reported on normal compilation?

Also note that the final error is the one reported in this PR.

[solardiz]

Did you see these errors reported on normal compilation?

I don't recall seeing them. We seem to use -Wall -Wno-stringop-truncation -Wno-format-overflow -Wno-format-truncation -Wno-deprecated-declarations -Wunused-but-set-variable -Wdate-time, so I'd expect to see warnings about unused variables, but then in this specific case we had:

               // unused
               (void) Flags;
               (void) Bitlen;
               (void) Reserved1;
               (void) Size;

which I guess suppressed them, even though the above was above the assignments.

I think I've just patched these warnings, please try re-running clang-tidy.

As to strcat, we have 60+ uses in the tree, and 230+ strcpy, so it'll need to be a tree-wide decision and effort if we want to never use those functions. This specific usage you found looks safe and it's after a mem_realloc of what looks like the correct size - but indeed it's time-consuming and error-prone to review these all like that. We have some strn* functions of our own in misc.c. In other projects I created - popa3d and blists - we have the concat function, which is safer to use in such cases. Maybe we should import and start using that one where appropriate.