Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve compiler warnings #597

Merged
merged 3 commits into from
Mar 5, 2020
Merged

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Mar 2, 2020

Compiling Themis with recent GCC and Clang versions produces new warnings. We are treating warnings as errors. Resolve the warnings to avoid build failues when we upgrade the compilers.

Commit summary

Suppress false positive warning

Recent versions of GCC are getting smarter, but here it has outsmarted itself. Drop the compiler a hint that a 'possible null pointer dereference' is in fact impossible in this case.

Do not "fix up" iterator ordering

Recent versions of GCC started printing a warning about those lines:

if (begin < end) {
    return input_buffer(&*begin, end - begin);
}

It says that we are "assuming pointer wraparound does not occur when comparing P +- C1 with P +- C2". In fact, we do assume that.

Here we try to be "helpful" and correct a possible mistake of swapping begin and end iterators. However, after thinking about it, this mistake is unlikely to happen. Moreover, we really rely on the user to provide correct iterators:

  1. They should point into the same container.
  2. They should point to valid elements of said container.
  3. The container must store its elements in contiguous memory.
  4. The "begin" iterator should come before the "end" one.

If any of these assumptions is broken, you get undefined behavior (most likely resuling in a crash caused by segmentation fault). We cannot verify these assumptions by simply checking that one address is lower than the other. Debug builds of STL may check that, but that's as good as you can get it in C++.

So stop doing that (un)helpful fixup. Remove the check and do not swap iterators, removing the cause of the warning too.

Initialize structs with memset() manually

In C, it is perfectly legal to zero-initialize structures with {0}, like this:

struct themis_scell_pbkdf2_context kdf = {0};

However, this is not (always) legal in C++. Certain versions of afl-clang are more C++-leaning and produce warnings about uninitialized fields even when compiling C code.

Well, okay, I don't want to fence this code with suppressions, so replace this initialization is explicit memset() calls.

Checklist

Recent versions of GCC are getting smarter, but here it has outsmarted
itself. Drop the compiler a hint that 'possible null pointer dereference'
is in fact impossible in this case.
Recent versions of GCC started printing a warning about those lines [1]:

    if (begin < end) {
        return input_buffer(&*begin, end - begin);
    }

It says that we are "assuming pointer wraparound does not occur when
comparing P +- C1 with P +- C2". In fact, we do assume that.

[1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=49283

Here we try to be "helpful" and correct a possible mistake of swapping
begin and end iterators. However, after thinking about it, this mistake
is unlikely to happen. Moreover, we really rely on the user to provide
correct iterators:

 1) They should point into the same container.
 2) They should point to valid elements of said container.
 3) The container must store its elements in contiguous memory.
 4) The "begin" iterator should come before the "end" one.

If any of these assumptions is broken, you get undefined behavior (most
likely resuling in a crash caused by segmentation fault). We cannot
verify these assumptions by simply checking that one address is lower
than the other. Debug builds of STL may check that, but that's as good
as you can get it in C++.

So stop doing that (un)helpful fixup. Remove the check and do not swap
iterators, removing the cause of the warning too.
In C, it is perfectly legal to zero-initialize structures with {0},
like this:

    struct themis_scell_pbkdf2_context kdf = {0};

However, this is not (always) legal in C++. Certain versions of
afl-clang are more C++-leaning and produce warnings about uninitialized
fields even when compiling C code.

Well, okay, I don't want to fence this code with suppressions,
so replace this initialization is explicit memset() calls.
@ilammy ilammy added core Themis Core written in C, its packages W-ThemisPP ⚔️ Wrapper: ThemisPP, C++ API labels Mar 2, 2020
@ilammy ilammy requested review from vixentael and Lagovas March 2, 2020 11:52
@ilammy ilammy mentioned this pull request Mar 2, 2020
9 tasks
Copy link
Collaborator

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ilammy ilammy merged commit a2a5cd1 into cossacklabs:master Mar 5, 2020
@ilammy ilammy deleted the compiler-warnings branch March 5, 2020 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Themis Core written in C, its packages W-ThemisPP ⚔️ Wrapper: ThemisPP, C++ API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants