Re: [PATCH bpf-next] bpftool: fix a bug in subskeleton code generation

[Yonghong Song <yhs@fb.com>]

On 3/21/22 7:31 AM, Daniel Borkmann wrote:
> On 3/20/22 4:20 AM, Yonghong Song wrote:
>> Compiled with clang by adding LLVM=1 both kernel and selftests/bpf
>> build, I hit the following compilation error:
>>
>> In file included from 
>> /.../tools/testing/selftests/bpf/prog_tests/subskeleton.c:6:
>>    ./test_subskeleton_lib.subskel.h:168:6: error: variable 'err' is 
>> used uninitialized whenever
>>        'if' condition is true [-Werror,-Wsometimes-uninitialized]
>>            if (!s->progs)
>>                ^~~~~~~~~
>>    ./test_subskeleton_lib.subskel.h:181:11: note: uninitialized use 
>> occurs here
>>            errno = -err;
>>                     ^~~
>>    ./test_subskeleton_lib.subskel.h:168:2: note: remove the 'if' if 
>> its condition is always false
>>            if (!s->progs)
>>            ^~~~~~~~~~~~~~
>>
>> The compilation error is triggered by the following code
>>          ...
>>          int err;
>>
>>          obj = (struct test_subskeleton_lib *)calloc(1, sizeof(*obj));
>>          if (!obj) {
>>                  errno = ENOMEM;
>>                  goto err;
>>          }
>>          ...
>>
>>    err:
>>          test_subskeleton_lib__destroy(obj);
>>          errno = -err;
>>          ...
>> in test_subskeleton_lib__open(). The 'err' is not initialized, yet it
>> is used in 'errno = -err' later.
>>
>> The fix is to remove 'errno = -err' since errno has been set properly
>> in all incoming branches.
> 
> If we remove this one here in which locations is it missing then? Do 
> these then
> need an extra errno = -err statement before they goto err?

Everything should be covered. The following are all 'goto err' returns:

         obj = (struct test_subskeleton_lib *)calloc(1, sizeof(*obj));
         if (!obj) {
                 errno = ENOMEM;
                 goto err;
         }
         s = (struct bpf_object_subskeleton *)calloc(1, sizeof(*s));
         if (!s) {
                 errno = ENOMEM;
                 goto err;
         }
	...
         s->vars = (struct bpf_var_skeleton *)calloc(10, sizeof(*s->vars));
         if (!s->vars) {
                 errno = ENOMEM;
                 goto err;
         }
	...

==> for all maps

         /* maps */
         s->map_cnt = 7;
         s->map_skel_sz = sizeof(*s->maps);
         s->maps = (struct bpf_map_skeleton *)calloc(s->map_cnt, 
s->map_skel_sz);
         if (!s->maps)
                 goto err;
==> calloc should set error number properly if failed.
	...

==> for all progs
	/* programs */
         s->prog_cnt = 1;
         s->prog_skel_sz = sizeof(*s->progs);
         s->progs = (struct bpf_prog_skeleton *)calloc(s->prog_cnt, 
s->prog_skel_sz);
         if (!s->progs)
                 goto err;
==> calloc should set error number properly if failed.
	
         err = bpf_object__open_subskeleton(s);
         if (err)
                 goto err;

	return obj;

==> bpf_object__open_subskeleton() in libbpf.c does set errno probably 
if 'err' is not 0.


> 
>> Cc: Delyan Kratunov <delyank@fb.com>
>> Fixes: 00389c58ffe9 ("00389c58ffe993782a8ba4bb5a34a102b1f6fe24")
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   tools/bpf/bpftool/gen.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
>> index 96bd2b33ccf6..7ba7ff55d2ea 100644
>> --- a/tools/bpf/bpftool/gen.c
>> +++ b/tools/bpf/bpftool/gen.c
>> @@ -1538,7 +1538,6 @@ static int do_subskeleton(int argc, char **argv)
>>               return obj;                        \n\
>>           err:                                \n\
>>               %1$s__destroy(obj);                    \n\
>> -            errno = -err;                        \n\
>>               return NULL;                        \n\
>>           }                                \n\
>>                                           \n\
>>
>