Created attachment 274945 [details] [review]
Patch v1, 0001

Created attachment 274946 [details] [review]
Patch v1, 0002

Comment on attachment 274945 [details] [review]
Patch v1, 0001

>+_g_value_set (GValue *dst, GValue *src)
>+{
>+	if (src) {

You need an else clause here, to clear dst if src was NULL.

>+		if (!G_IS_VALUE (dst))
>+			g_value_init (dst, type);
>+		else if (G_VALUE_TYPE (dst) != type) {
>+			g_value_unset (dst);
>+			g_value_init (dst, type);
>+		} else
>+			g_value_reset (dst);

You can simplify this to:

		if (G_IS_VALUE (dst))
			g_value_unset (dst);
		g_value_init (dst, type);

Comment on attachment 274946 [details] [review]
Patch v1, 0002

>When accepting the values, check for the proper GType and g_warn
>on error.

Why? Doesn't gvalue already do that?

>+		if (type != G_VALUE_TYPE (src)) {
>+			g_warn_if_reached ();
>+			return;
>+		}

g_return_if_fail (type == G_VALUE_TYPE (src));

(In reply to comment #3)
> (From update of attachment 274945 [details] [review])
> >+_g_value_set (GValue *dst, GValue *src)
> >+{
> >+	if (src) {
> 
> You need an else clause here, to clear dst if src was NULL.

you mean

 if (src) {
    ...
 } else
     g_value_unset (dst);

? 

This would actually change the behaviour to what it was before...

Before

-	val = g_hash_table_lookup (config, NM_VPN_PLUGIN_CONFIG_MTU);
-	if (val) {
-		g_value_init (&priv->mtu, G_VALUE_TYPE (val));
-		g_value_copy (val, &priv->mtu);
-	}

it would just skip over missing values (leaving the previous value).


> >+		if (!G_IS_VALUE (dst))
> >+			g_value_init (dst, type);
> >+		else if (G_VALUE_TYPE (dst) != type) {
> >+			g_value_unset (dst);
> >+			g_value_init (dst, type);
> >+		} else
> >+			g_value_reset (dst);
> 
> You can simplify this to:
> 
>         if (G_IS_VALUE (dst))
>             g_value_unset (dst);
>         g_value_init (dst, type);

true

(In reply to comment #4)
> (From update of attachment 274946 [details] [review])
> >When accepting the values, check for the proper GType and g_warn
> >on error.
> 
> Why? Doesn't gvalue already do that?

really? If you would a wrong type, then it would stay in the hashset. Maybe the error would be noted later or maybe not (looking at src/vpn-manager/nm-vpn-connection.c:816)

It makes sense to check for wrong type at an early place.



> >+		if (type != G_VALUE_TYPE (src)) {
> >+			g_warn_if_reached ();
> >+			return;
> >+		}
> 
> g_return_if_fail (type == G_VALUE_TYPE (src));

Hm, I wanted to g_warn, instead of g_return (which is less critical)...

(In reply to comment #5)
> This would actually change the behaviour to what it was before...

> it would just skip over missing values (leaving the previous value).

But that code was written assuming that set_config() was only going to be called once (hence the failure to g_value_unset()).

And if you look at what that code does, it's just to make sure that certain values from Config get copied into Ip4Config as well; so if the new Config hash doesn't include that value, then the new Ip4Config hash shouldn't either.

(In reply to comment #6)
> > g_return_if_fail (type == G_VALUE_TYPE (src));
> 
> Hm, I wanted to g_warn, instead of g_return (which is less critical)...

g_return_if_fail() is a g_critical(), which is more critical than g_warn*

(In reply to comment #7)
> (In reply to comment #5)
> > This would actually change the behaviour to what it was before...
> 
> > it would just skip over missing values (leaving the previous value).
> 
> But that code was written assuming that set_config() was only going to be
> called once (hence the failure to g_value_unset()).
> 
> And if you look at what that code does, it's just to make sure that certain
> values from Config get copied into Ip4Config as well; so if the new Config hash
> doesn't include that value, then the new Ip4Config hash shouldn't either.

I could reproduce this warning easily with the openvpn plugin by sending -USR1 to the openvpn process. So, maybe there is a bug there.

Are you sure that the assumption holds?


> (In reply to comment #6)
> > > g_return_if_fail (type == G_VALUE_TYPE (src));
> > 
> > Hm, I wanted to g_warn, instead of g_return (which is less critical)...
> 
> g_return_if_fail() is a g_critical(), which is more critical than g_warn*

Oh yes. I mean, I wanted only to warn, not critically.

(In reply to comment #8)
> I could reproduce this warning easily with the openvpn plugin by sending -USR1
> to the openvpn process. So, maybe there is a bug there.

We already *know* there's a bug there; it's the bug that you're fixing in this patch. But you're only fixing part of it.

> Are you sure that the assumption holds?

As the person who wrote the code in question, yes.

Reworked both patches and pushed to th/bgo728791_vpn_SetConfig.

@Dan, could you take another look?


Note, that now the case of resetting a property a second time again causes a g_warn(), but the value gets unset.



(In reply to comment #9)
> (In reply to comment #8)
> > I could reproduce this warning easily with the openvpn plugin by sending -USR1
> > to the openvpn process. So, maybe there is a bug there.
> 
> We already *know* there's a bug there; it's the bug that you're fixing in this
> patch. But you're only fixing part of it.

You mean, the other part of the bug is in nm-openvpn?

(In reply to comment #10)
> Reworked both patches and pushed to th/bgo728791_vpn_SetConfig.
> 
> @Dan, could you take another look?
> 
> 
> Note, that now the case of resetting a property a second time again causes a
> g_warn(), but the value gets unset.

A clearer explanation would be:

"If a new value is empty/not-given but was previously given/not-empty, a warning will be raised."

but the code allows you to overwrite a previously set value with a new non-empty value without a warning.

It took me a minute here to realize that this branch is really about ensuring that things work when SetConfig() is called by the VPN plugin helper (that gets executed by vpnc/openvpn/etc when the VPN connect is successful) a *second* or later time, like with Thomas' nm-openvpn fixes for up-restart.  Might be worth clarifying that in the commit message, like:

----
libnm-glib-vpn: fix resetting IP config during additional SetConfig() calls

When receiving updated VPN IP configuration from the helper after the initial
connect event, the library overwrites the already initialized GValue fields
by calling g_value_init() again. This is an error and causes the following
warning:

(nm-openvpn-service:27645): GLib-GObject-WARNING **: gvalue.c:183: cannot initialize GValue with type gchararray, the value has already been initialized as gchararray
----

I still don't understand, is this a bug of the user/caller (nm-openvpn)?

[comment #7]
  >> But that code was written assuming that set_config() was only going to be
  >> called once (hence the failure to g_value_unset()).


In that case the caller must be fixed and we should g_warn always also in case
of [comment #11]
  >> but the code allows you to overwrite a previously set value with a new
  >> non-empty value without a warning.


Otherwise, no g_warn should be raised, because the user did nothing wrong.

There is one bug: the change that was made to nm_vpn_plugin_set_config() and nm_vpn_plugin_set_ip4_config() in 960c1ae8 implicitly assumed that they could each only ever be called once in any given process. Which is not true.

There are two manifestations of this bug: (1) if the second (or third, etc) call to nm_vpn_plugin_set_config() tries to update banner, tundev, gateway, or mtu, then there will be a g_warning because it will try to re-g_value_init() a GValue that is already initialized. (2) if the second (third, etc) call to nm_vpn_plugin_set_config() *doesn't* update banner, tundev, gateway, or mtu, then the next call to nm_vpn_plugin_set_ip4_config() will copy the OLD values of those properties from the earlier nm_vpn_plugin_set_config() call into the hash table emitted in the IP4_CONFIG signal, when it actually should be leaving them unset, like they were in the corresponding CONFIG signal.

Understood.

Repushed (non-ff).

Reworded commit message according to comment 11, and added two fixup commits.

Hmm, the latest code in nm_vpn_plugin_set_config() will leave a previous priv->gateway if the (second/third/etc) call doesn't pass a gateway.  It should g_clear_value(&priv->gateway); if NM_VPN_PLUGIN_CONFIG_EXT_GATEWAY isn't passed in the hash.

(In reply to comment #15)
> Hmm, the latest code in nm_vpn_plugin_set_config() will leave a previous
> priv->gateway if the (second/third/etc) call doesn't pass a gateway.  It should
> g_clear_value(&priv->gateway); if NM_VPN_PLUGIN_CONFIG_EXT_GATEWAY isn't passed
> in the hash.

Rebased fixup which fixes this.

Two additional fixup commits change to code to use variadic arguments. I think it's nicer and shorter.

The first commit and its fixup (which mistakenly references the second commit rather than the first) look good.

I don't think we need the second commit; we'll already get warnings in the daemon anyway if the VPN helper sends the wrong types (since, eg, g_value_get_string() will spew if !G_VALUE_HOLDS_STRING()). And it's weird to check the types of 4 of the properties, but not all the other ones...

(In reply to comment #17)
> The first commit and its fixup (which mistakenly references the second commit
> rather than the first) look good.
> 
> I don't think we need the second commit; we'll already get warnings in the
> daemon anyway if the VPN helper sends the wrong types (since, eg,
> g_value_get_string() will spew if !G_VALUE_HOLDS_STRING()). And it's weird to
> check the types of 4 of the properties, but not all the other ones...

It will, unless glib is compiled with G_DISABLE_CHECKS.
I think, if NM allows a g_return_*() to happen, it is a bug and not something that ever should happen on user-input.

E.g. see src/vpn-manager/nm-vpn-connection.c, process_generic_config().

Is this worth fixing too? I pushed a commit on top of th/bgo728791_vpn_SetConfig, which catches some of these cases and fixes some leaks.


I think the second commit is useful, because it produces a g_warning early on, when the user does the wrong thing, not only later in the deamon. The deamon anyway has to do any validation independently. I don't mind the mentioned weirdness :)

(In reply to comment #18)
> (In reply to comment #17)
> > I don't think we need the second commit; we'll already get warnings in the
> > daemon anyway if the VPN helper sends the wrong types (since, eg,
> > g_value_get_string() will spew if !G_VALUE_HOLDS_STRING()). And it's weird to
> > check the types of 4 of the properties, but not all the other ones...
> 
> It will, unless glib is compiled with G_DISABLE_CHECKS.
> I think, if NM allows a g_return_*() to happen, it is a bug

(You don't ever need to think about G_DISABLE_CHECKS. As you say, when the condition checked by a g_return_*() is FALSE, then that indicates a bug. But it's a bug whether G_DISABLE_CHECKS is defined or not, so there is no reason to include G_DISABLE_CHECKS in your reasoning.)

> and not something that ever should happen on user-input.

Note that there is no user input here; the data is all generated by the VPN service helper program, based on data it receives from the VPN command-line program. So, if the data types are wrong, it *does* indicate a bug (in, eg, nm-openvpn-service-helper).

(Arguably, return-if-fails probably shouldn't occur across process boundaries though. But in that case, nm-vpn-connection still needs to check the data types no matter what nm-vpn-plugin does, because they are in different processes as well.)

> I think the second commit is useful, because it produces a g_warning early on,
> when the user does the wrong thing, not only later in the deamon. The deamon
> anyway has to do any validation independently. I don't mind the mentioned
> weirdness :)

Again, it's not "the user does the wrong thing", it's "the service helper program has a bug in it". And it's not "early on", because the data gets passed to the daemon almost immediately after it's received by the plugin. And at any rate, you are type-checking 4 of the arguments, but leaving the other 23 un-checked, and not checking that all of the arguments passed in are actually defined, and not checking that the mandatory arguments are present, etc.

And there's no reason for NMVpnPlugin to do any of that; it just receives a hash, and passes it on. NMVpnConnection is what actually interprets the hash, and knows what information does and doesn't need to be there, so that's where it makes sense to check it.

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)

> > I think the second commit is useful, because it produces a g_warning early on,
> > when the user does the wrong thing, not only later in the deamon. The deamon
> > anyway has to do any validation independently. I don't mind the mentioned
> > weirdness :)
> 
> Again, it's not "the user does the wrong thing", it's "the service helper
> program has a bug in it". And it's not "early on", because the data gets passed
> to the daemon almost immediately after it's received by the plugin. And at any
> rate, you are type-checking 4 of the arguments, but leaving the other 23
> un-checked, and not checking that all of the arguments passed in are actually
> defined, and not checking that the mandatory arguments are present, etc.
> 
> And there's no reason for NMVpnPlugin to do any of that; it just receives a
> hash, and passes it on. NMVpnConnection is what actually interprets the hash,
> and knows what information does and doesn't need to be there, so that's where
> it makes sense to check it.

The commits adds some validation to libnm-glib. So, it is closer to the "consumer" of the library who does something wrong -- I see the word "user" is confusing here.

It might not be the earliest point where the library could validate anything, but it's relatively "early-on", and still at the place where the consumer tries to set the wrong value -- contrary to a validation failure one process-boundary away.

Anyway, we can also not validate in the library, let's just drop the commit
(I just pushed a new commit "fixup! libnm-glib: check GValue type in vpn_plugin_set_config (SetConfig)", to fix a bug there).



I pushed another commit:

  "vpn: cleanup receiving VPN parameters and check for GValue types"

> libnm-glib: check GValue type in vpn_plugin_set_config (SetConfig)

Instead of "(GType) 0" how about G_TYPE_INVALID?

Since we're always expecting at least one GType, is the first outer "if (type != G_TYPE_INVALID)" useful?  Wouldn't this be simpler and achieve the same result?

		va_start (ap, src);
		type = va_arg (ap, GType);
		while (type != G_TYPE_INVALID) {
			if (type == src_type)
				goto valid_type;
			type = va_arg (ap, GType);
		}
		g_warn_if_reached ();
valid_type:
		va_end (ap);


> vpn: cleanup receiving VPN parameters and check for GValue types

I'd save some horizontal space and

#define INVALID_ARG_FMT "VPN connection '%s' has invalid argument %s"
#define INVALID_ARG_ARGS(key) nm_connection_get_id (priv->connection),key

nm_log_dbg (LOGD_VPN, INVALID_ARG_FMT, INVALID_ARG_ARGS(NM_VPN_PLUGIN_CONFIG_BANNER));

Though that looks kinda ugly with all the capitals everywhere.  So maybe just:

nm_log_dbg (LOGD_VPN, INVALID_ARG_FMT, nm_connection_get_id (priv->connection), , NM_VPN_PLUGIN_CONFIG_BANNER);

static void
__g_value_set (GValue *dst, GValue *src, ...)
#define _g_value_set(dst, src, ...)    __g_value_set (dst, src, __VA_ARGS__, (GType) 0);
{

I'd move the #define above the function definition and remove the trailing semicolon.

(In reply to comment #21)
> > libnm-glib: check GValue type in vpn_plugin_set_config (SetConfig)
> 
> Instead of "(GType) 0" how about G_TYPE_INVALID?

Done


> Since we're always expecting at least one GType, is the first outer "if (type
> != G_TYPE_INVALID)" useful?  Wouldn't this be simpler and achieve the same
> result?
> 
>         va_start (ap, src);
>         type = va_arg (ap, GType);
>         while (type != G_TYPE_INVALID) {
>             if (type == src_type)
>                 goto valid_type;
>             type = va_arg (ap, GType);
>         }
>         g_warn_if_reached ();
> valid_type:
>         va_end (ap);

Possible, which saves two LOC. But then it always needs at least one type argument (which currently is indeed always the case). But the function was also useful beyond the type-checking... I would keep it.


> > vpn: cleanup receiving VPN parameters and check for GValue types
> 
> I'd save some horizontal space and
> 
> #define INVALID_ARG_FMT "VPN connection '%s' has invalid argument %s"
> #define INVALID_ARG_ARGS(key) nm_connection_get_id (priv->connection),key
> 
> nm_log_dbg (LOGD_VPN, INVALID_ARG_FMT,
> INVALID_ARG_ARGS(NM_VPN_PLUGIN_CONFIG_BANNER));
> 
> Though that looks kinda ugly with all the capitals everywhere.  So maybe just:
> 
> nm_log_dbg (LOGD_VPN, INVALID_ARG_FMT, nm_connection_get_id (priv->connection),
> , NM_VPN_PLUGIN_CONFIG_BANNER);

I solved it a bit differently, 

+#define LOG_INVALID_ARG(property) \
+    nm_log_dbg (LOGD_VPN, "VPN connection '%s' has invalid argument %s", \
+                          nm_connection_get_id (priv->connection), property)

how is that?




(In reply to comment #22)
> static void
> __g_value_set (GValue *dst, GValue *src, ...)
> #define _g_value_set(dst, src, ...)    __g_value_set (dst, src, __VA_ARGS__,
> (GType) 0);
> {
> 
> I'd move the #define above the function definition and remove the trailing
> semicolon.

Done





How about danw's suggestion to completely drop commit
  "libnm-glib: check GValue type in vpn_plugin_set_config (SetConfig)"
?

(In reply to comment #23)
> > > vpn: cleanup receiving VPN parameters and check for GValue types
> > 
> > I'd save some horizontal space and
> > 
> > #define INVALID_ARG_FMT "VPN connection '%s' has invalid argument %s"
> > #define INVALID_ARG_ARGS(key) nm_connection_get_id (priv->connection),key
> > 
> > nm_log_dbg (LOGD_VPN, INVALID_ARG_FMT,
> > INVALID_ARG_ARGS(NM_VPN_PLUGIN_CONFIG_BANNER));
> > 
> > Though that looks kinda ugly with all the capitals everywhere.  So maybe just:
> > 
> > nm_log_dbg (LOGD_VPN, INVALID_ARG_FMT, nm_connection_get_id (priv->connection),
> > , NM_VPN_PLUGIN_CONFIG_BANNER);
> 
> I solved it a bit differently, 
> 
> +#define LOG_INVALID_ARG(property) \
> +    nm_log_dbg (LOGD_VPN, "VPN connection '%s' has invalid argument %s", \
> +                          nm_connection_get_id (priv->connection), property)
> 
> how is that?

Looks fine to me.

> How about danw's suggestion to completely drop commit
>   "libnm-glib: check GValue type in vpn_plugin_set_config (SetConfig)"

Yeah, he does have a point... it's data coming from a root process, and it's programmer error if the types are wrong (since they are specified in the NetworkManagerVPN.h header too).  So I guess we don't need to validate them there, just in nm-vpn-connection.c.

Merged two commits as

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=f53a35e030cb7b03515a8470f81a9d10ff72179e

and dropped the other one.

Created attachment 277896 [details] [review]
[PATCH] rejected commit to check GValue type in libnm-glib

For the record, here the rejected patch