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 720433 - E: geary 64bit-portability-issue src/engine/imap/response/imap-server-data.vala:156
E: geary 64bit-portability-issue src/engine/imap/response/imap-server-data.va...
Status: RESOLVED FIXED
Product: geary
Classification: Other
Component: build
0.4.x
Other Linux
: Normal normal
: 0.6.0
Assigned To: Geary Maintainers
Geary Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-12-13 22:03 UTC by Dominique Leuenberger
Modified: 2013-12-14 00:00 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Dominique Leuenberger 2013-12-13 22:03:10 UTC
While packaging geary 0.4.2 / 0.4.3 for openSUSE, I ran into this error, raised by the build-policy checker:

E: geary 64bit-portability-issue src/engine/imap/response/imap-server-data.vala:156

The respective compiler output that triggers the error in the end:

[  195s] /home/abuild/rpmbuild/BUILD/geary-0.4.2/src/engine/imap/response/imap-server-data.vala:156:13: warning: assignment makes integer from pointer without a cast [enabled by default]
[  195s]              results.add(get_as_string(ctr).as_int(0));
[  195s]              ^
[  195s] /home/abuild/rpmbuild/BUILD/geary-0.4.2/src/engine/imap/response/imap-server-data.vala:156:5: warning: passing argument 2 of 'gee_collection_add' makes pointer from integer without a cast [enabled by default]
[  195s]              results.add(get_as_string(ctr).as_int(0));
[  195s]      ^

Looking at the code:

>for (int ctr = 2; ctr < size; ctr++) {
>results.add(get_as_string(ctr).as_int(0));
  => looks over complicated and error prone?

It seems simply to convert ctr to string, then back to int, ensuring it's at least 0?

so why not:
for (int ctr = 2; ctr < size; ctr++) {
  results.add(ctr < 0 ? 0 : ctr);

This seems to be doing the same, without actually converting and also without triggering the above error.

(btw: with ctr=2 and then only ctr++: in what universe can ctr be < 0?)
Comment 1 Jim Nelson 2013-12-13 22:19:02 UTC
(In reply to comment #0)
> It seems simply to convert ctr to string, then back to int, ensuring it's at
> least 0?

That's not what's it's doing.

Note that method in question is a part of class ServerData, which ultimately inherits from ListParameter.  ListParameter is, as it sounds, a list of parameters which can be of various types.

    results.add(get_as_string(ctr).as_int(0));

get_as_string() is calling to the parent class ListParameter, fetching a parameter at index (ctr) and asking it to be cast to a StringParameter, if possible.  (It it can't, it throws an Error.)  as_int(0) then requests the StringParameter return an integer representation of itself, which again is not always possible and could result in an Error being thrown (we're not being real strict on this step, although we should be).

In other words, ctr is not being converted to a string and then back to an int.  ctr is in an index into an array of elements which are converted to a StringParameter and then, finally, an int.

Part of the complication here is that IMAP specifies a few ways that data may be transmitted.  All this looks complicated, but it's to deal with situations where servers do unexpected things, which happens more often than you would think with IMAP.

The 64-bit warning is something worth worrying about, however.  I suspect the origin is not get_as_string() or to_int() but that the final int is being added to a Gee.List<int> and that there's some 64-bit issues there.

If you write a simple Vala program, something like this:

void main() {
  Gee.List<int> list = new Gee.ArrayList<int>();
  list.add(1);
}

does your policy build checker warn similarly?
Comment 2 Dominique Leuenberger 2013-12-13 22:50:26 UTC
(In reply to comment #1)
> If you write a simple Vala program, something like this:
> 
> void main() {
>   Gee.List<int> list = new Gee.ArrayList<int>();
>   list.add(1);
> }
> 
> does your policy build checker warn similarly?

To complete the trail.. as we worked on IRC on this topic:
=> no: above code does not give an error.

For reference, we also patched geary as follows to work around the error:

diff -ur geary-0.4.3/src/engine/imap/response/imap-server-data.vala geary-0.4.3.patched/src/engine/imap/response/imap-server-data.vala
--- geary-0.4.3/src/engine/imap/response/imap-server-data.vala	2013-12-13 00:07:29.000000000 +0100
+++ geary-0.4.3.patched/src/engine/imap/response/imap-server-data.vala	2013-12-13 23:34:44.994386337 +0100
@@ -153,7 +153,7 @@
         
         Gee.List<int> results = new Gee.ArrayList<int>();
         for (int ctr = 2; ctr < size; ctr++) {
-            results.add(get_as_string(ctr).as_int(0));
+            int a = get_as_string(ctr).as_int(0); results.add(a);
         }
         
         return results;


That works as well
Comment 3 Jim Nelson 2013-12-13 23:29:27 UTC
Ultimately the bug lies in Vala: bug #720437.  However, we can work around this using a patch as suggested above.

My question is, is this bug preventing you from shipping/distributing Geary?  Can this wait for the next unstable release, or 0.6 stable?
Comment 4 Jim Nelson 2013-12-14 00:00:17 UTC
commit:32deb02