libkmod: Do not set errno in libkmod-index

[Maaxxs]

There are two implementations for the index. The mmap index does not use errno at all. Which implementation is used, is based on the usage of kmod_load_resources and whether it succeeds or not. Therefore, callers using libkmod cannot rely on useful information in errno.

Part of the reason why we investigated this is the fact that an invalid magic value read by index_file_open leads to a NULL return value with errno set to 0. This happens because read_u32s sets errno to 0 on success, while index_file_open initializes errno to EINVAL before performing any read operations.

No proof of concept because errno isn't checked 🙃

@stoeckmann

[evelikov]

callers using libkmod cannot rely on useful information in errno.

There are zero mentions of errno in the API documentation, so external users should not relying on it. On the other hand, there are multiple places within libkmod that do depend on it.

Have you checked all direct and indirect users of the changed functions don't check errno?

[lucasdemarchi]

Therefore, callers using libkmod cannot rely on useful information in errno.

errno is an interface with libc. You can't call a library method and then inspect errno - the things the library will call from libc will change according to configuration/improvements/kernel version etc. Our interface for error code is the return value from the functions.

As for the code change here, if it can be accepted, I will leave it to @stoeckmann to take a look.

[stoeckmann]

Have you checked all direct and indirect users of the changed functions don't check errno?

It took us some time, until we noticed a fact we described in the second paragraph (edit: Our first attempt of the second paragraph. It never made it into the PR. Oops). I guess @Maaxxs was right and we should have illustrated it with a code example. So ...

The main entry point for FILE-based index processing is index_file_open. It is always surrounded by an if-statement like

if (ctx->indexes[index_number] != NULL) {
    ... memory-mapped index handling happens here ...
} else {
    ... FILE-based index handling happens here ...
}

This means that we can focus solely on the else-block, because outside of the if-statement, the code must handle both cases. And since we know that memory-mapped index handling doesn't set errno at all, everything outside of the if-statement can be ignored for validation.

Within these else-blocks, we haven't found any errno checks. They are all located within libkmod.c:

1. index_file_open -> index_searchwild -> index_file_close
2. index_file_open -> index_search -> index_file_close
3. index_file_open -> index_dump -> index_file_close

This implies that these else-blocks only call FILE-based index functions without doing anything fancy by calling other functions.

As far as I can be considered an independent reviewer of a PR which Max and I created together, I would say it's okay. :) And hopefully, this explanation helps someone else to look at it again. It's fortunately not that much code outside of libkmod-index.c although one would expect otherwise at first.

[stoeckmann]

Aside: Looking around, we have other instances of similar errno misuse - libkmod-builtin.c, libkmod-elf.c libkmod-file-zlib.c, libkmod-file.c, libkmod-module.c - can we get some patches for those?

Looking forward to your contributions. At least it sounds like you investigated these files already. I mean, you did more than just grep -l 'errno = ' libkmod/*, right? :D

[lucasdemarchi]

Not sure what y'all are talking about. There's nothing wrong on using errno inside the library and setting errno may be needed depending on what we are doing. Any user calling a libkmod function and then checking errno is not properly checking for error - errno is a libc interface and libc is used by both libkmod and the caller.

[lucasdemarchi]

Applied, thanks. Although in the commit message I mentioned I also removed the inline, I actually forgot about that so it stayed.