microsoft / ebpf-for-windows

eBPF implementation that runs on top of Windows
MIT License
2.91k stars 227 forks source link

Revert C6011 override in external\Analyze.external.ruleset once upstream issue is fixed #309

Closed Alan-Jowett closed 3 years ago

Alan-Jowett commented 3 years ago

Revert C6011 override in external\Analyze.external.ruleset once upstream issue is fixed.

Due to https://github.com/vbpf/ebpf-verifier/issues/239 there is an override that make C6011 a warning in the external submodules. Once this is resolved, remove this overide.

dthaler commented 3 years ago

The ebpf-verifier fix in 239 was not sufficient. The following warning remains:

D:\a\ebpf-for-windows\ebpf-for-windows\external\ebpf-verifier\src\asm_files.cpp(131): error C6011: Dereferencing NULL pointer 'maps_section'.

Offhand it looks like a false positive. Code:

            if (reloc.get_entries_num() && !maps_section) {
                throw std::runtime_error(string("Can't find any maps sections in file ") + path);
            }

            for (ELFIO::Elf_Xword i = 0; i < reloc.get_entries_num(); i++) {
                if (reloc.get_entry(i, offset, symbol, type, addend)) {
                    ebpf_inst& inst = prog.prog[offset / sizeof(ebpf_inst)];

                    string symbol_name;
                    ELFIO::Elf64_Addr symbol_value{};
                    unsigned char symbol_bind{};
                    unsigned char symbol_type{};
                    ELFIO::Elf_Half symbol_section_index{};
                    unsigned char symbol_other{};
                    ELFIO::Elf_Xword symbol_size{};

                    symbols.get_symbol(symbol, symbol_name, symbol_value, symbol_size, symbol_bind, symbol_type,
                                       symbol_section_index, symbol_other);

                    // Only perform relocation for symbols located in the maps section.
                    if (symbol_section_index != maps_section->get_index()) {

The first check will throw if maps_section is null and there are reloc entries. And the warning is generated inside the loop that is only traversed if there are reloc entries. I suspect the analyzer doesn't know whether reloc.get_entries_num() can change in between the two checks. Maybe saving it in a local variable will make the analyzer happy.

Alan-Jowett commented 3 years ago

image

It looks like that is the case based on the detailed trace.

Alan-Jowett commented 3 years ago

Created PR https://github.com/vbpf/ebpf-verifier/pull/242 to get this resolved.