Merge lp:~alexmurray/whoopsie/whoopsie into lp:whoopsie

[Alex Murray]

[Iain Lane (laney)]

Hi Alex,

Thanks for this. I've pushed 706 to lp:whoopsie. While reviewing 707, though, I was wondering why we don't use GString in utils.c? It seems like relying on GLib's better tested code (whoopsie already depends on GLib) is a sensible thing to do - do you agree?

I had a go at converting utils.c over in this merge proposal: https://code.launchpad.net/~laney/whoopsie/gstring/+merge/369880

Would you care to review that and let me know what you think? I've requested you on that one.

If you think it's more risky then we can still go with what you've got here - but I'm currently of the opinion that it seems better to do less memory wrangling ourselves if we can achieve that.

(Related side note: the report parsing in src/whoopsie.c is scary)

review: Needs Information

[Alex Murray (alexmurray)]

Yep I agree - much better to rely on GLib's string handling.

[Iain Lane (laney)]

Great, thanks!

Unmerged revisions

707. By Alex Murray on 2019-07-09

src/utils.c: Don't use strncpy where strcpy will suffice

The code is already very careful to allocate a buffer large enough for the
length of the filename extension so there is no need to use strncpy to
append this to the new filename - instead can just use strcpy.

With the previous code, whoopsie would FTBFS against gcc 8.3.0 due to the
following error as a result of this use of strncpy - so changing to strcpy
fixes the build:

In file included from /usr/include/string.h:494,
                 from src/utils.c:19:
In function ‘strncpy’,
    inlined from ‘change_file_extension’ at src/utils.c:92:5,
    inlined from ‘change_file_extension’ at src/utils.c:67:1:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ output truncated before terminating nul copying as many bytes from a string as its length [-Werror=stringop-truncation]
  106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/utils.c: In function ‘change_file_extension’:
src/utils.c:89:15: note: length computed here
   89 | ext_len = strlen (extension);
      | ^~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

=== modified file 'src/utils.c'
--- src/utils.c	2018-11-20 21:44:17 +0000
+++ src/utils.c	2019-07-09 02:18:52 +0000
@@ -89,7 +89,7 @@
     ext_len = strlen (extension);
     new_path = g_malloc (len + ext_len + 1);
     memcpy (new_path, path, len);
-    strncpy (new_path + len, extension, ext_len);
+    strcpy (new_path + len, extension);
     /* Ensure the new string is NUL terminated */
     new_path[len + ext_len] = '\0';
     return new_path;

=== modified file 'src/whoopsie.c'
--- src/whoopsie.c	2018-11-19 23:27:48 +0000
+++ src/whoopsie.c	2019-07-09 02:18:52 +0000
@@ -374,8 +374,8 @@
     gchar* value_p = NULL;
     GError* err = NULL;
     gchar* end = NULL;
-    int value_length;
-    int value_pos;
+    size_t value_length;
+    size_t value_pos;
 
     g_return_val_if_fail (report_path, NULL);