GNOME Bugzilla – Bug 312105
[PATCH] Fix crash adding an attachment
Last modified: 2005-08-08 00:49:46 UTC
Version details: 2.3.4 Distribution/Version: All versions This is a bit tricky (and easy at the same time). I'm sure it doesn't crash on Linux, or it would have been noticed before. However, on FreeBSD, if you add an attachment (any attachment) to a new message, Balsa crashes in gtk_list_store_set(). The problem is that Balsa tries to fit the attachment's size into a gint. However, on FreeBSD, the stat.st_size member is of type off_t which is analagous to a uint64_t. Therefore, the attached patch stores the attachment's size as a guint64.
Created attachment 49995 [details] [review] Fix crash when adding an attachment
Thanks for the report! As you note, this is tricky...with your patch, my compile fails because the g_strdup_printf on line 2680 has a mismatch between the guint64 argument and the %d format specifier. I can fix it with %lld, but that works for me only because sizeof(long long int) == sizeof(guint64). That may not be portable. I believe that long int (== glong) may be the largest type that has a G_TYPE_* macro (and can therefore be specified in gtk_tree_store_new() and can be specified in a printf format specifier ("%ld"). The following patch builds on my box--could you check it out? It casts st_size to (glong), so if you attached a really huge file, the size would be truncated, but it's used only for printing, so it shouldn't be fatal.
Created attachment 50039 [details] [review] proposed substitute patch (against cvs)
I don't know how mine worked with that printf. I completely missed it. You're right that this patch will work (in that it compiles, and doesn't crash), but you're still casting an off_t to a long on 32-bit FreeBSD. Therefore, you will lose some precision (though anyone attaching a file > 2 GB should be shot). But as you say, this is only for printing purposes. Oh, and I was mistaken before. An off_t is an int64_t not a uint64_t. I misread sys/types.h. And G_TYPE_* does support the INT64 type, but I agree that %lld is not portable. That's why FreeBSD 5.X and higher offers %zd. I think your new patch is the way to go.
Thanks for testing it! Yes, a >2GB attachment would be unthinkable with current pathways, so there's no substantive issue. I'm sure we'll have a portable way to handle this before it matters! I'll commit to cvs. Peter
Created attachment 50065 [details] [review] incremental patch Here's a better fix: use a float to store st_size in the tree-list, and cast it to (int) when we know it's small. Also plugs a memory leak. This patch requires the previous one, which is already in cvs.
Resolving as FIXED--feel free to reopen if you find any remaining problems.