After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 764132 - [review] escape arguments when exporting Openvpn configuration and cleanup [th/export-bgo764132]
[review] escape arguments when exporting Openvpn configuration and cleanup [t...
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: VPN: openvpn
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2016-03-24 11:30 UTC by Thomas Haller
Modified: 2016-03-29 11:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Description Thomas Haller 2016-03-24 11:30:48 UTC 
When writing a openvpn configuration file during export, we must properly handle special characters (escaping).


Thereby, also refactor the code. Especially, don't have a pre-ample where we lookup all the values first and write them later. Just do it all in one place.
Comment 1 Beniamino Galvani 2016-03-25 17:46:53 UTC 
> properties: refactor export code to properly escape strings

+static const char *
+escape_arg (const char *value, char **buf)

If the string contains a single quote, backslash and double quotes are
escaped, but spaces are not. I think we have to either escape them or
enclose the string in double quotes.


+                  : (tmp_proto
+                     ? (!strcmp (tmp_proto, "udp")

nm_streq ?


> properties: refactor do_export() to first write the file to a GString buffer
> properties: use g_file_sets_contents() to write file in do_export()

Maybe squash these two?	Otherwise the comment in the first commit
about error checking is misleading (error checking is added later).

+       args_write_line (f,
+                        "dev",
+                        device ?:
+                            (device_type ?:
+                                (nm_streq0 (nm_setting_vpn_get_data_item (s_vpn, NM_OPENVPN_KEY_TAP_DEV), "yes")
+                                    ? "tun" : "tap")));

should be:  ? "tap" : "tun")));

The rest LGTM.
Comment 2 Thomas Haller 2016-03-25 19:49:50 UTC 
(In reply to Beniamino Galvani from comment #1)
> > properties: refactor export code to properly escape strings
> 
> +static const char *
> +escape_arg (const char *value, char **buf)
> 
> If the string contains a single quote, backslash and double quotes are
> escaped, but spaces are not. I think we have to either escape them or
> enclose the string in double quotes.

Good catch. Previous version was badly messed up. How about now?

> +                  : (tmp_proto
> +                     ? (!strcmp (tmp_proto, "udp")
> 
> nm_streq ?

fixup!


> > properties: refactor do_export() to first write the file to a GString buffer
> > properties: use g_file_sets_contents() to write file in do_export()
> 
> Maybe squash these two?	Otherwise the comment in the first commit
> about error checking is misleading (error checking is added later).

I reworded the commit message of the first commit. Ok?

> +       args_write_line (f,
> +                        "dev",
> +                        device ?:
> +                            (device_type ?:
> +                                (nm_streq0 (nm_setting_vpn_get_data_item
> (s_vpn, NM_OPENVPN_KEY_TAP_DEV), "yes")
> +                                    ? "tun" : "tap")));
> 
> should be:  ? "tap" : "tun")));

fixup!



Repushed