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 711546 - utf8: report allocation error
utf8: report allocation error
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2013-11-06 13:14 UTC by Marc-Andre Lureau
Modified: 2013-12-03 15:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
utf8: report allocation error (3.86 KB, patch)
2013-11-06 13:14 UTC, Marc-Andre Lureau
reviewed Details | Review
utf8: report allocation error (3.87 KB, patch)
2013-11-15 18:01 UTC, Marc-Andre Lureau
needs-work Details | Review
utf8: report allocation error (3.97 KB, patch)
2013-11-23 14:27 UTC, Marc-Andre Lureau
committed Details | Review
gutf8: use g_try_malloc_n (2.66 KB, patch)
2013-11-26 16:48 UTC, Marc-Andre Lureau
committed Details | Review

Description Marc-Andre Lureau 2013-11-06 13:14:51 UTC
Make some of the conversion functions a bit more friendly to allocation
failure.

Even though the glib policy is to abort() on allocation failure by
default, it can be quite helpful to return an allocation error for
functions already providing a GError.

I needed a safer g_utf16_to_utf8() to solve crash on big clipboard
operations with win32, related to rhbz#1017250 (and coming gdk handling
bug).
Comment 1 Marc-Andre Lureau 2013-11-06 13:14:53 UTC
Created attachment 259070 [details] [review]
utf8: report allocation error
Comment 2 Colin Walters 2013-11-06 22:10:13 UTC
Review of attachment 259070 [details] [review]:

Urgh.  This is going to add a whole new level of inconsistency to our APIs.  Are we going to say change g_file_read() to also return an error if say g_object_new(G_TYPE_LOCAL_FILE_INPUT_STREAM) fails?  I doubt it...

I could relate a bit to adding a new g_try_utf16_to_utf8() API.  

What is your application going to *do* with this error anyways?  Are you going to pop up a dialog box like "Your clipboard is too big!"?

This needs input from other GLib maintainers too.
Comment 3 Marc-Andre Lureau 2013-11-06 23:38:50 UTC
(In reply to comment #2)
> Review of attachment 259070 [details] [review]:
> 
> Urgh.  This is going to add a whole new level of inconsistency to our APIs.

Well, as long as the function was already explicitely returning NULL on error, it shouldn't be too much of inconsistency.

> Are we going to say change g_file_read() to also return an error if say
> g_object_new(G_TYPE_LOCAL_FILE_INPUT_STREAM) fails?  I doubt it...

That's a reasonably sized object of fixed size. Contrast with the utf8 conversion function, doing arbitrary allocation size.

> I could relate a bit to adding a new g_try_utf16_to_utf8() API.  

That's an option, I just don't think we should add so many _try function, when we already explicitely have return error API in existing functions.

> What is your application going to *do* with this error anyways?  Are you going
> to pop up a dialog box like "Your clipboard is too big!"?

Nothing, if user attempts to paste data coming from another app that is too big, it should not crash, and do nothing.

There are already various gdk clipboard path doing this (at least on win32 side, which I have been reviewing)
Comment 4 Allison Karlitskaya (desrt) 2013-11-07 14:02:44 UTC
 glib/gutf8.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 58 insertions(+), 15 deletions(-)

The diffstat from this patch reads like a interactive guide as to why we like to abort on malloc failures.

We have no shortage of utf8 mangling APIs, but the problem for you appears to be that we lack good utf16 operations that allow you to work with buffers that you've allocated for yourself.  Considering that this is for use on win32, I'm assuming that Windows provides such APIs....

Couldn't you implement utf16-to-utf8 by the following approach:

 - scan for unicode characters in utf16 input using windows API
 - g_unichar_to_utf8(NULL) to get the size of each utf8 character
 - allocate the buffer yourself, using _try_ variants of malloc
 - use g_unichar_to_utf8() again into that buffer.


If at all possible, I'd really prefer the above approach to trying to deal with malloc() failing in our general-purpose APIs.
Comment 5 Marc-Andre Lureau 2013-11-11 16:12:00 UTC
(In reply to comment #4)
>  glib/gutf8.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 58 insertions(+), 15 deletions(-)
> 
> The diffstat from this patch reads like a interactive guide as to why we like
> to abort on malloc failures.

I can make it much smaller if you prefer, I didn't know we were guided by diffstat.

> 
> We have no shortage of utf8 mangling APIs, but the problem for you appears to
> be that we lack good utf16 operations that allow you to work with buffers that
> you've allocated for yourself.  Considering that this is for use on win32, I'm
> assuming that Windows provides such APIs....

I don't need a buffer that I allocated myself, I am just looking for a somewhat safer utf8_to_utf16 (one that can fail to allocate destination buffer)

I would like this function to be used in the win32 gdk, but that doesn't mean this is win32-specific, nor gdk specific.

I was looking for the simple most obvious solution, one that doesn't require API change, or new implementation.

> Couldn't you implement utf16-to-utf8 by the following approach:
> 
>  - scan for unicode characters in utf16 input using windows API
>  - g_unichar_to_utf8(NULL) to get the size of each utf8 character
>  - allocate the buffer yourself, using _try_ variants of malloc
>  - use g_unichar_to_utf8() again into that buffer.

Sure, I can reinvent the wheel, but I am concerned that such approach raises disparity and the chances to do it wrong or with peculiarities.

> If at all possible, I'd really prefer the above approach to trying to deal with
> malloc() failing in our general-purpose APIs.

When there is already error reporting, and the change is not invasive (one condition branch for ex), I don't think this rule should be apply to the expense of other projects, who will have to workaroud or reimplement basic functionality.
Comment 6 Dan Winship 2013-11-15 16:24:10 UTC
(In reply to comment #2)
> Urgh.  This is going to add a whole new level of inconsistency to our APIs. 
> Are we going to say change g_file_read() to also return an error if say
> g_object_new(G_TYPE_LOCAL_FILE_INPUT_STREAM) fails?  I doubt it...

If memory is so low that g_object_new(G_TYPE_LOCAL_FILE_INPUT_STREAM) fails, then g_error_new() is almost certainly going to fail as well, so we wouldn't be able to report it anyway.

Marc-Andre's patch is only useful in the case where you do have plenty of free memory, but @len is just ridiculously large. And there's precedent for using g_try_malloc() and returning an error if it fails in cases like that (eg, g_file_get_contents()).

So I think the patch makes sense, although there should probably be a new GConvertError value to distinguish this from other failures.


(Specifically, I think returning an "out of memory" error can make sense if the API needs to make allocations whose sizes are determined by external data not entirely in the program's control, and either (a) the API can also return non-memory-related GErrors, so the caller needs to check for errors anyway, or (b) out-of-memory errors are sufficiently likely that it's worth adding a GError to the API just for that one case.)
Comment 7 Colin Walters 2013-11-15 16:55:06 UTC
(In reply to comment #6)
> And there's precedent for using
> g_try_malloc() and returning an error if it fails in cases like that (eg,
> g_file_get_contents()).

The fact that there is precedent here in GLib, combined with your argument about externally-controlled sizes has convinced me.

Agreed as well that we want a new error code.
Comment 8 Marc-Andre Lureau 2013-11-15 18:01:52 UTC
Created attachment 259931 [details] [review]
utf8: report allocation error

Make some of the conversion functions a bit more friendly to allocation
failure.

Even though the glib policy is to abort() on allocation failure by
default, it can be quite helpful to return an allocation error for
functions already providing a GError.

I needed a safer g_utf16_to_utf8() to solve crash on big clipboard
operations with win32, related to rhbz#1017250 (and coming gdk handling
bug).
Comment 9 Matthias Clasen 2013-11-23 05:02:20 UTC
Review of attachment 259931 [details] [review]:

::: glib/gutf8.c
@@ +852,3 @@
 
+  if (!(result = try_malloc (sizeof (gunichar) * (n_chars + 1), error)))
+      goto err_out;

I don't like assignments buried inside conditions. Can we make this

result = try_malloc (..., error);
if (result == NULL)
  goto err_out;

please ? Same below
Comment 10 Marc-Andre Lureau 2013-11-23 14:27:25 UTC
Created attachment 261296 [details] [review]
utf8: report allocation error

Make some of the conversion functions a bit more friendly to allocation
failure.

Even though the glib policy is to abort() on allocation failure by
default, it can be quite helpful to return an allocation error for
functions already providing a GError.

I needed a safer g_utf16_to_utf8() to solve crash on big clipboard
operations with win32, related to rhbz#1017250 (and coming gdk handling
bug).
Comment 11 Matthias Clasen 2013-11-24 02:54:01 UTC
Review of attachment 261296 [details] [review]:

looks good now, thanks
Comment 12 Marc-Andre Lureau 2013-11-25 11:09:40 UTC
Attachment 261296 [details] pushed as d6a19d2 - utf8: report allocation error
Comment 13 Christian Persch 2013-11-25 14:55:06 UTC
+static gpointer
+try_malloc (gsize n_bytes, GError **error)
+{
+    gpointer ptr = g_try_malloc (n_bytes);
[...]
+  result = try_malloc (sizeof (gunichar) * (n_chars + 1), error);

We have g_try_malloc_n() which is careful to avoid overflows; I think this code should use it instead of doing this multiplication itself.
Comment 14 Marc-Andre Lureau 2013-11-26 16:48:21 UTC
Created attachment 262879 [details] [review]
gutf8: use g_try_malloc_n

As recommended by Christian Persch.
Comment 15 Christian Persch 2013-11-26 16:50:11 UTC
Looks good to me. 

But the +1 resp. +4 might overflow too :-)
Comment 16 Matthias Clasen 2013-11-26 16:57:02 UTC
Review of attachment 262879 [details] [review]:

looks good
Comment 17 Marc-Andre Lureau 2013-12-03 15:11:04 UTC
Attachment 262879 [details] pushed as b2bf13c - gutf8: use g_try_malloc_n