[gjs: 1/2] arg-cache: Always use an unsigned int mask for flags

[Philip Chimento <pchimento src gnome org>]

commit 1716c8667677f57f90be13aca7b7f6d90e0a5ece
Author: Simon McVittie <smcv debian org>
Date:   Sat Aug 22 23:25:41 2020 +0100

    arg-cache: Always use an unsigned int mask for flags
    
    When retrieving flags from the GIArgumentInfo in
    set_return_ffi_arg_from_giargument(), we use the v_int member of the
    union (arguably we ought to use the v_uint member for flags, but we
    don't), so to avoid undefined behaviour from type-punning we need to
    store flags via that same member.
    
    In particular, even if we somehow convince ourselves that a flags type
    can have values outside the 32-bit range, it would be incorrect to
    store them in the v_int64 or v_uint64 member, because when we read
    them back out it'll be via the v_int member. That's formally undefined
    behaviour, but in practice will access the first 32 bits of the 64-bit
    integer, which is the same as (real_value & 0xffff'ffff) on
    little-endian CPUs, but gives a meaningless result on big-endian.
    Sure enough, after 1f3984c9 the unit tests start failing on s390x,
    which is 64-bit big-endian.
    
    GLib's GFlagsClass.mask and GFlagsValue.value are of type guint
    (equivalent to unsigned int), so it can't possibly represent flags
    outside the range of guint as a GType anyway. To be consistent with
    that (and have reasonable unsigned arithmetic), go via an unsigned
    integer.
    
    The corresponding fix for enum types was done as a side-effect of
    8b2927b0 "arg-cache: Save space in enum bounds".
    
    Fixes: 1f3984c9 "arg-cache: extend to handle interface types too"
    Resolves: https://gitlab.gnome.org/GNOME/gjs/-/issues/341
    Signed-off-by: Simon McVittie <smcv debian org>

 gi/arg-cache.cpp | 18 +++++++++++-------
 gi/arg-cache.h   |  2 +-
 2 files changed, 12 insertions(+), 8 deletions(-)
---
diff --git a/gi/arg-cache.cpp b/gi/arg-cache.cpp
index ab8c0f84..e7ed2a65 100644
--- a/gi/arg-cache.cpp
+++ b/gi/arg-cache.cpp
@@ -572,11 +572,10 @@ static bool gjs_marshal_flags_in_in(JSContext* cx, GjsArgumentCache* self,
         return false;
     }
 
-    if (self->contents.flags_mask <= G_MAXUINT32)
-        gjs_arg_set<unsigned, GI_TYPE_TAG_INTERFACE>(arg, number);
-    else
-        gjs_arg_set(arg, uint64_t(number));
-
+    // We cast to unsigned because that's what makes sense, but then we
+    // put it in the v_int slot because that's what we use to unmarshal
+    // flags types at the moment.
+    gjs_arg_set<int, GI_TYPE_TAG_INTERFACE>(arg, static_cast<unsigned>(number));
     return true;
 }
 
@@ -1292,8 +1291,13 @@ static void gjs_arg_cache_build_flags_mask(GjsArgumentCache* self,
     int n = g_enum_info_get_n_values(enum_info);
     for (int i = 0; i < n; i++) {
         GjsAutoValueInfo value_info = g_enum_info_get_value(enum_info, i);
-        uint64_t value = uint64_t(g_value_info_get_value(value_info));
-        mask |= value;
+        int64_t value = g_value_info_get_value(value_info);
+        // From the docs for g_value_info_get_value(): "This will always be
+        // representable as a 32-bit signed or unsigned value. The use of
+        // gint64 as the return type is to allow both."
+        // We stuff both into an unsigned, int-sized field, matching the
+        // internal representation of flags in GLib (which uses guint).
+        mask |= static_cast<unsigned>(value);
     }
 
     self->contents.flags_mask = mask;
diff --git a/gi/arg-cache.h b/gi/arg-cache.h
index a7223920..5cf93145 100644
--- a/gi/arg-cache.h
+++ b/gi/arg-cache.h
@@ -93,7 +93,7 @@ struct GjsArgumentCache {
             uint32_t enum_min;
             uint32_t enum_max;
         } enum_type;
-        uint64_t flags_mask;
+        unsigned flags_mask;
 
         // string / filename
         bool string_is_filename : 1;