Applying Facebook's static analysis tool Infer

Applying Facebook's static analysis tool Infer

[Lukas Bulwahn]

Hi,

this is a first patch of my work to apply Facebook's static analysis tool
Infer (http://fbinfer.com) on the Linux kernel source code.

I am starting the analysis and patching on the kernel scripts and tools.
Further analysis of the kernel will then probably require more tweaking of
the static analysis tool to handle the kernel source code appropriately.

As I will continue to send patches following the same commit-message scheme
to issues that Infer finds, let me know if further output information from
the tool should be included or condensed in the commit message.

Lukas

[PATCH] fixdep: free memory on second error path of do_config_file

[Lukas Bulwahn]

Commit dee81e988674 ("fixdep: faster CONFIG_ search") introduces the memory
leak when `map = mmap(...)` was replaced with `map = malloc(...)` and
`read(fd, map, ...)`. It introduces a new second error path, which does not
free the allocated memory for `map`. We now correct that behavior and free
`map` before the do_config_file() function returns.

Facebook's static analysis tool Infer (http://fbinfer.com) found this
memory leak:

  scripts/basic/fixdep.c:297: error: MEMORY_LEAK
    memory dynamically allocated by call to `malloc()` at line 290, \
    column 8 is not reachable after line 297, column 3.

Fixes: dee81e988674 ("fixdep: faster CONFIG_ search")

Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
 scripts/basic/fixdep.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index bbf62cb..131c450 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -296,6 +296,7 @@ static void do_config_file(const char *filename)
 	if (read(fd, map, st.st_size) != st.st_size) {
 		perror("fixdep: read");
 		close(fd);
+		free(map);
 		return;
 	}
 	map[st.st_size] = '\0';
-- 
2.7.4

Re: [SIL2review] [PATCH] fixdep: free memory on second error path of do_config_file

[Nicholas Mc Guire]

On Thu, Dec 14, 2017 at 08:54:10PM +0100, Lukas Bulwahn wrote:
> Commit dee81e988674 ("fixdep: faster CONFIG_ search") introduces the memory
> leak when `map = mmap(...)` was replaced with `map = malloc(...)` and
> `read(fd, map, ...)`. It introduces a new second error path, which does not
> free the allocated memory for `map`. We now correct that behavior and free
> `map` before the do_config_file() function returns.
> 
> Facebook's static analysis tool Infer (http://fbinfer.com) found this
> memory leak:
> 
>   scripts/basic/fixdep.c:297: error: MEMORY_LEAK
>     memory dynamically allocated by call to `malloc()` at line 290, \
>     column 8 is not reachable after line 297, column 3.
> 
> Fixes: dee81e988674 ("fixdep: faster CONFIG_ search")
> 
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Reviewed-by: Nicholas Mc Guire <der.herr@hofr.at>

> ---
>  scripts/basic/fixdep.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> index bbf62cb..131c450 100644
> --- a/scripts/basic/fixdep.c
> +++ b/scripts/basic/fixdep.c
> @@ -296,6 +296,7 @@ static void do_config_file(const char *filename)
>  	if (read(fd, map, st.st_size) != st.st_size) {
>  		perror("fixdep: read");
>  		close(fd);
> +		free(map);

 This looks reasonable but actually it is not clear why do_config_file()
should return at all if read fails as the read error would go unnoticed
in the current code and allow the build to continue. so this probably
should be an exit(2) here and not a return which would then take care
of the free() anyway.

 Atleast I do not see the rational to allow continuation if the read 
failed as the file should not be empty nor a mismatch with st.st_size
expected. If it were due to a EINTR then it still should terminate as
EINTR was not handled and we thus could miss a valid dependency.

 Note: this probably also should be applied to the if (!map) condition
before as well, as at that point it is known that map > 0 and a malloc()
failure would allow skipping parse_config_file() for a valid config.

>  		return;
>  	}
>  	map[st.st_size] = '\0';
> -- 
> 2.7.4
> 
> _______________________________________________
> SIL2review mailing list
> SIL2review@lists.osadl.org
> https://lists.osadl.org/mailman/listinfo/sil2review

Re: [SIL2review] [PATCH] fixdep: free memory on second error path of do_config_file

[Masahiro Yamada]

2017-12-15 17:23 GMT+09:00 Nicholas Mc Guire <der.herr@hofr.at>:
> On Thu, Dec 14, 2017 at 08:54:10PM +0100, Lukas Bulwahn wrote:
>> Commit dee81e988674 ("fixdep: faster CONFIG_ search") introduces the memory
>> leak when `map = mmap(...)` was replaced with `map = malloc(...)` and
>> `read(fd, map, ...)`. It introduces a new second error path, which does not
>> free the allocated memory for `map`. We now correct that behavior and free
>> `map` before the do_config_file() function returns.
>>
>> Facebook's static analysis tool Infer (http://fbinfer.com) found this
>> memory leak:
>>
>>   scripts/basic/fixdep.c:297: error: MEMORY_LEAK
>>     memory dynamically allocated by call to `malloc()` at line 290, \
>>     column 8 is not reachable after line 297, column 3.
>>
>> Fixes: dee81e988674 ("fixdep: faster CONFIG_ search")
>>
>> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Reviewed-by: Nicholas Mc Guire <der.herr@hofr.at>
>
>> ---
>>  scripts/basic/fixdep.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
>> index bbf62cb..131c450 100644
>> --- a/scripts/basic/fixdep.c
>> +++ b/scripts/basic/fixdep.c
>> @@ -296,6 +296,7 @@ static void do_config_file(const char *filename)
>>       if (read(fd, map, st.st_size) != st.st_size) {
>>               perror("fixdep: read");
>>               close(fd);
>> +             free(map);
>
>  This looks reasonable but actually it is not clear why do_config_file()
> should return at all if read fails as the read error would go unnoticed
> in the current code and allow the build to continue. so this probably
> should be an exit(2) here and not a return which would then take care
> of the free() anyway.

I agree.

This should be exit(2).

You can also remove close() as well since
the operation system will close it anyway.



>  Atleast I do not see the rational to allow continuation if the read
> failed as the file should not be empty nor a mismatch with st.st_size
> expected. If it were due to a EINTR then it still should terminate as
> EINTR was not handled and we thus could miss a valid dependency.
>
>  Note: this probably also should be applied to the if (!map) condition
> before as well, as at that point it is known that map > 0 and a malloc()
> failure would allow skipping parse_config_file() for a valid config.

I agree.  We should change this too.



-- 
Best Regards
Masahiro Yamada

Re: [SIL2review] [PATCH] fixdep: free memory on second error path of do_config_file

[Lukas Bulwahn]

On Mon, 18 Dec 2017, Masahiro Yamada wrote:

> 2017-12-15 17:23 GMT+09:00 Nicholas Mc Guire <der.herr@hofr.at>:
>> On Thu, Dec 14, 2017 at 08:54:10PM +0100, Lukas Bulwahn wrote:
>>> Commit dee81e988674 ("fixdep: faster CONFIG_ search") introduces the memory
>>> leak when `map = mmap(...)` was replaced with `map = malloc(...)` and
>>> `read(fd, map, ...)`. It introduces a new second error path, which does not
>>> free the allocated memory for `map`. We now correct that behavior and free
>>> `map` before the do_config_file() function returns.
>>>
>>> Facebook's static analysis tool Infer (http://fbinfer.com) found this
>>> memory leak:
>>>
>>>   scripts/basic/fixdep.c:297: error: MEMORY_LEAK
>>>     memory dynamically allocated by call to `malloc()` at line 290, \
>>>     column 8 is not reachable after line 297, column 3.
>>>
>>> Fixes: dee81e988674 ("fixdep: faster CONFIG_ search")
>>>
>>> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
>> Reviewed-by: Nicholas Mc Guire <der.herr@hofr.at>
>>
>>> ---
>>>  scripts/basic/fixdep.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
>>> index bbf62cb..131c450 100644
>>> --- a/scripts/basic/fixdep.c
>>> +++ b/scripts/basic/fixdep.c
>>> @@ -296,6 +296,7 @@ static void do_config_file(const char *filename)
>>>       if (read(fd, map, st.st_size) != st.st_size) {
>>>               perror("fixdep: read");
>>>               close(fd);
>>> +             free(map);
>>
>>  This looks reasonable but actually it is not clear why do_config_file()
>> should return at all if read fails as the read error would go unnoticed
>> in the current code and allow the build to continue. so this probably
>> should be an exit(2) here and not a return which would then take care
>> of the free() anyway.
>
> I agree.
>
> This should be exit(2).
>
> You can also remove close() as well since
> the operation system will close it anyway.
>
>
>
>>  Atleast I do not see the rational to allow continuation if the read
>> failed as the file should not be empty nor a mismatch with st.st_size
>> expected. If it were due to a EINTR then it still should terminate as
>> EINTR was not handled and we thus could miss a valid dependency.
>>
>>  Note: this probably also should be applied to the if (!map) condition
>> before as well, as at that point it is known that map > 0 and a malloc()
>> failure would allow skipping parse_config_file() for a valid config.
>
> I agree.  We should change this too.
>

I created a new patch with your requested changes, but I did not consider 
it a second version of this patch by the time of writing. You can find the 
new patch at:

v1: https://patchwork.kernel.org/patch/10126547/
v2: https://patchwork.kernel.org/patch/10128297/

This patch here is now superseded by the new patch. So, this patch here 
can be abandoned.

Lukas