Fix keyfile parsing of wireguard config when the prefix of allowed IPs is omited #366
Conversation
| @@ -511,7 +511,18 @@ parse_tunnels(GKeyFile* kf, NetplanNetDefinition* nd) | |||
| for (int i = 0; allowed_ips_split[i] != NULL; i++) { | |||
| gchar* ip = allowed_ips_split[i]; | |||
| if (g_strcmp0(ip, "")) { | |||
| gchar* address = g_strdup(ip); | |||
| gchar* address = NULL; | |||
There was a problem hiding this comment.
nitpick (not-blocking): IMHO it's actually better to leave it uninitialized here.
In this case, the NULL value is an invalid value for address, as we're always assuming it be a valid string. Now, we know that, but the compiler doesn't, and so it won't necessarily help us when we're modifying the code later on and use it before its actual "semantic" initialization. Whereas just declaring it means the compiler will loudly complain, because it's pretty good at tracking those.
Yes it's a hypothetical, and the entire thing is highly subjective, I can easily imagine slyon disagreeing with me on that, hence my marking this as a nitpick. Feel free to disregard and mark this as resolved if you're not convinced ;)
There was a problem hiding this comment.
I see your point and I think it's valid. But then, we probably shouldn't initialized anything with default values that are not considered valid in the context...
In any case, in this specific context, not initializing address apparently doesn't bother the compiler. Because we pass it to g_array_append_val which is a macro that will call g_array_append_vals passing a pointer to address (g_array_append_vals (a, &(v), 1)). I guess the compiler assumes that as we are passing a pointer to a variable, it will be initialized inside the function we called or something like that.
There was a problem hiding this comment.
Way more simple than that: the compiler sees that no matter which codepath you take, address is written before it is read :). From its PoV there's no way g_array_append_vals would initialize address, as it can only mutate whatever it points to, not the pointer itself (which is what is uninitialized).
However, you're right in saying that gcc will not issue an uninitialized warning if you pass a pointer to your uninitialized variable to a function, unless it can conclusively prove it will read it before writing.
Take this sample:
#include <stdio.h>
void f(int* out);
int main()
{
int i;
f(&i);
printf("%d", i);
return 0;
}
void f(int* out)
{
printf("%d", *out);
}At default optimization level, it won't complain, even if you enable all warnings (-Wall -Wextra). However, if you enable -O3 (and keep -Wall), it'll loudly tell you about the uninitialized access, most likely because there's an inlining pass before the analysis for this warning is done.
Yes, my school of thought is that we should never have variables with semantically invalid values, unless there's a good reason (make the compiler shut up is a good reason ;-) )
There was a problem hiding this comment.
Yeah, leaving it to the compiler to do different things depending on what level of optimization you are using can lead to amazing surprises... Like this problem I had some time ago when my system failed to boot after an upgrade :)
Not trying to compare both situations here but I think that this kind of problem should probably be caught by tests (and static analysis)...
The parameter "regenerate" enable the caller to check or not if the regenerated keyfile matches the input. This will be used by tests that parse keyfiles with implicit configuration. In this cases, the regenerated file will include this configuration and will not match the input. For example, the Wireguard property allowed-ips accept IPs without a prefix, but the prefix /32 is implicitly used as it is required by Wireguard.
Network Manager doesn't fail if an IP in the allowed-ips list doesn't have a prefix. In this case, WG will default to /32. As the WG's documentation says the prefix must be used, Netplan will fail the validation step if it's not present. To be able to parse a keyfile and generate a YAML that will not fail validation, let's append a /32 automatically to the value if it doesn't have a prefix defined by the user.
daniloegea
force-pushed
the
fix_wireguard_parsing
branch
from
7fde170
to
23451ab
Compare
|
Not trying to compare both situations here but I think that this kind of problem should probably be caught by tests (and static analysis)...
I wonder if UBSan would catch this. Also I wonder if we couldn't add a
UBSan pass similar to your ASan work?
Anyway, I'll just merge this as it is.
|
Description
Network Manager allows entering IPs without prefixes. Although WG docs says the format a.b.c.d/prefix should be used, it will accept only the IP address and default the prefix to /32. This change will append a /32 to the allowed IPs that don't have a prefix found in the keyfile.
Checklist
make checksuccessfully.make check-coverage).