#64 Results of the static analysis tool FB Infer on zip source files

[Tomáš Beránek]

Hi,

as a part of my Bachelor's thesis I have been analyzing packages on Fedora
with static analysis tool called Facebook Infer. When analyzing package
zip-3.0-27.fc33 Infer produced some interesting errors. I have checked them
manually and it seems, that they are real (not false positives).
I will summarize them into a single bug report:

1) unix/unix.c:256: error: Null Dereference
pointer t last assigned on line 236 could be null and is dereferenced at line 256, column 10.

In function 'ex2in' (unix/unix.c:228) null dereference can happen on the variable 't',
if the parameter 'x' is, for example, '//' or '//host'. I'm not sure if 'x' can have
these values, but since there is a check (for example, unix/unix.c:252) for it,
I assume, that it is possible. However, if this happens, then the initial NULL value
stored in 't' will not be overwritten and will be dereferenced at unix/unix.c:256.

2) crypt.c:265: error: Dead Store
The value written to &res (type int) is never used.

This DEAD_STORE leads to a dereference on an uninitialized pointer. If malloc fails
in the function 'readlocal', then 'localz' variable will not be initialized. Since
the return value of the function 'readlocal' is not checked, the program will continue
until unix/unix.c:273 where dereference happens and most likely will fail.

3) crypt.c:354: error: Dead Store
The value written to &res (type int) is never used.

A similar issue as #2.

4) unix/unix.c:469: error: Memory Leak
memory dynamically allocated at line 467 by call to malloc, is not freed after the last access at line 469, column 7.

While checking other Infer reports I have noticed, that you try to free every
allocated memory even if the program fails. So I assume, that this memory leak
is not intentional. If the first malloc (unix/unix.c:467) succeeds and the second
malloc (unix/unix.c:469) fails, then the first one will not be freed before exit.

I haven't managed to find your active repository, otherwise, I would have created a pull
request with a fix to some of the errors mentioned above to save you some time.

Thank you in advance for any feedback.

All the best,
Tomáš Beránek

[Ed Gordon]

Sorry for the delay getting back to you.

Where did you download this copy of Zip? Just want to confirm we are looking at the same package. Note that end providers, like Fedora, tend to edit our base packages before including them in their distributions.

That said, we've made considerable modifications to Zip. Beta version Zip 3.1d should be on this site. Version Zip 3.1e hopefully should be hopefully posting in a month or so and should be close to being released as Zip 3.1. I'll check the latest internal beta to see if these issues have been addressed, but I should compare to Fedora's release to see the issues in context.

Thanks!

[Tomáš Beránek]

The analyzed package was zip-3.0-27.fc33 (default version on Fedora 33 from the official repository https://mirrors.fedoraproject.org/mirrorlist?repo=fedora-33&arch=x86_64). I wasn't able to find version zip31d on this site, only zip31c which seems to be the latest modified. I have checked the issues against this version and they are still present, but line numbers are a bit different. Updated line numbers:

1) unix/unix.c:343: error: Null Dereference
When i was looking at the issue again, i noticed that for x == "//" the NULL
dereference won't happen, but for x == "//host" or "//host/share" will. I made
a mistake while copying my notes.

4) unix/unix.c:557: error: Memory Leak
Allocation: 554
Return without free: 557

I also found another possible issue:

5) fileio.c:1295: error: Null Dereference
The function 'localtime' can return NULL and the value is dereferenced at the
next line.