(In reply to comment #0)
> The size of one of my gstreamer elements (an object based on gobject) ended up
> exceeding 64K and that went on without me noticing.

Yeah, don't do that. If you have a large buffer (array) per element instance, or something, allocate it separately and point to it instead.

This is impossible to fix if you're using g_type_register_static() or GTypePlugin, because in this case the assignment to 16-bit struct members is done in your code, and the truncation has already happened; on the other hand, in this case your compiler has enough information that it should be able to tell you, at compile time, that truncation will always occur.

The patch I'm about to attach adds a check if you're using g_type_register_static_simple(), which means G_DEFINE_TYPE and related macros will have the check.

Created attachment 198127 [details] [review]
Warn when classes/instances exceed the allowed size, if possible

It's impossible to check this if the library user is using
g_type_register_static, but in that case their compiler should hopefully
warn about the truncation. This fixes it for G_DEFINE_TYPE and friends,
at least.

Review of attachment 198127 [details] [review]:

Why can't we check this in g_type_register_static?  Wouldn't

g_return_val_if_fail (type_info->instance_size <= G_MAXUINT16, G_TYPE_INVALID)

work?

Oh sorry, truncation.  Duh.  Original patch looks good to me, thanks!

Created attachment 198205 [details] [review]
Document that types are limited to 64 KiB

(In reply to comment #4)
> Oh sorry, truncation.  Duh.  Original patch looks good to me, thanks!

I pushed it to master for 2.31.0, commit 5bed8317ea0dc615b02f34bde9b5dfa7374c5600.

(In reply to comment #1)
> (In reply to comment #0)
> > The size of one of my gstreamer elements (an object based on gobject) ended up
> > exceeding 64K and that went on without me noticing.
> 
> Yeah, don't do that. If you have a large buffer (array) per element instance,
> or something, allocate it separately and point to it instead.
> 
> This is impossible to fix if you're using g_type_register_static() or
> GTypePlugin, because in this case the assignment to 16-bit struct members is
> done in your code, and the truncation has already happened; on the other hand,
> in this case your compiler has enough information that it should be able to
> tell you, at compile time, that truncation will always occur.
> 
> The patch I'm about to attach adds a check if you're using
> g_type_register_static_simple(), which means G_DEFINE_TYPE and related macros
> will have the check.

Thanks Simon.

I fully understand where the issue is coming from. Sometimes it's hard to control the size of the object as there could be pieces that come from different people (even external teams), which could at their own wisdom increase the object size dramatically having no idea of the 16 bit limitation.
That's exactly what happened to me in this particular case.

I'll do as much verification on my side as I can and I will take as much help from the compiler and from the object system as I can get.

Regards,
-- Max Paklin.

(In reply to comment #7)
> Sometimes it's hard to
> control the size of the object as there could be pieces that come from
> different people (even external teams), which could at their own wisdom
> increase the object size dramatically having no idea of the 16 bit limitation.

I can see the problem. I recommend pointing to those of these pieces that aren't ABI-guaranteed to have a fixed size, rather than "inlining" them into your GObject instance struct:

    /* .h, bad */
    typedef struct {
        GObject parent_instance;
        OtherThing thing;
    } BadObject;

    /* .h, good */
    typedef struct {
        GObject parent_instance;
        OtherThing *thing;
    } GoodObject;

or better still, use GType private data:

    /* .h */
    typedef struct {
        GObject parent_instance;
        GoodObjectPrivate *priv;
    } GoodObject;

    /* .c */
    typedef struct {
        OtherThing *thing;
    } GoodObjectPrivate;

    void good_object_class_init (GoodObjectClass *cls)
    {
        g_type_class_add_private (cls, sizeof (GoodObjectPrivate));
    }

    void good_object_init (GoodObject *self)
    {
       self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self, GOOD_TYPE_OBJECT,
           GoodObjectPrivate);
    }

(Using private data adds an extra layer of indirection, but encapsulates the OtherThing into your implementation, meaning that if OtherThing breaks ABI, you don't necessarily break ABI too.)

The best checks I could add have now been committed to master; the remaining patch here documents the 64 KiB limit.

(In reply to comment #8)
>     /* .h, good */
>     typedef struct {
>         GObject parent_instance;
>         OtherThing *thing;
>     } GoodObject;

Simon,

Thanks for the explanation. I appreciate you taking the time.

What you described is exactly what I did to work around this 16 bit limitation.
I didn't go through using private data although that popped up as an option. I saw GStreamer using this technique.

While it's workable, it's really inconvenient.
Extra indirection is not an issue here. The issue is that I have to always check for OtherThing being properly initialized (!= NULL at least) before I access it.
This is the consequence of object_init() being void. If there were a way to indicate that the object failed to initialize, everything would be fine.

However the way things stand today I have to not forget to have this extra check or else things will break under heave load when the memory shortage is possible. What's worse whoever ends up supporting my code has to be aware of this as well.

I am using GStreamer, which puts an extra layer on top complicating things even further.

Do I miss anything? Is there is a proper way to handle this?
Thanks!

(In reply to comment #9)
> The issue is that I have to always
> check for OtherThing being properly initialized (!= NULL at least) before I
> access it.

If the only reason that it might be NULL is running out of memory, you should read <http://blog.ometer.com/2008/02/04/out-of-memory-handling-d-bus-experience/> before continuing. The extra code to handle OOM mentioned there significantly impedes maintenance of D-Bus by forcing everything to be transactional - and it ends up needing to make more allocations overall! (It often has to allocate the maximum amount of memory it might possibly need, because by the time it knows whether it actually needs that much memory, it has already had side-effects, making it too late to fail gracefully.)

On modern Linux (and most other OSs), malloc will not return NULL unless your process has an artificially small memory resource-limit, due to "overcommit" behaviour. If you run out of real RAM, performance will be poor as everything gets paged to and from swap, and if the kernel discovers that it cannot satisfy a previous request for memory due to overcommitting, it will try to work out "which process's fault it is", and kill that process to get memory back; both are entirely transparent to your process, except for reduced performance (when swapping) and the possibility of being killed (when memory is completely exhausted).

As a result, attempting to handle out-of-memory on small allocations is mostly a waste of time, and g_malloc() handles it by terminating the process.

> However the way things stand today I have to not forget to have this extra
> check or else things will break under heave load when the memory shortage is
> possible.

GObject instances are allocated with either g_malloc() or g_slice_malloc(), so failure to allocate a new instance terminates the process; inlining the struct into your GObject and hence making the instance larger (as in BadObject above) makes you *more* fragile under memory shortage, if anything.

> This is the consequence of object_init() being void. If there were a way to
> indicate that the object failed to initialize, everything would be fine.

If you really need failable construction, investigate GInitable, which has a failable pseudo-constructor. g_object_new() still can't fail, but it is considered to be an error to use a GInitable without first calling g_initable_init() on it, and that can fail; if it does, the only valid thing to do with that object is to unref it. GAsyncInitable is similar, but its pseudo-constructor is asynchronous.

> As a result, attempting to handle out-of-memory on small allocations is mostly
> a waste of time, and g_malloc() handles it by terminating the process.

This is probably an acceptable tradeoff (most programmers don't handle errors correctly as you mentioned) in desktop environment. My case is different. Sorry for not mentioning it before; I should've started with that.
I am running Linux in embedded environment on the box that has limited memory and no paging (frequently no disk).

Out of memory condition is real in my case and the one that I have to handle. The box supports multimedia streaming sessions in a different formats. Sometimes I can guesstimate how many there could be, sometimes I can't.

When I can't, it could be because the formats vary in their memory requirements significantly. The "easiest" could take 20 times less memory compared to the the most demanding one. To complicate things even more, I frequently don't know in advance what kind of stream I will be serving, thus I don't know what the bit rate is going to be.
Furthermore, bit rate can change dynamically based on the request from the client.

The only sensible thing to do is to allow the session to be created and then see if enough memory exists to support it at runtime. Crashing the process just because N+1th session cannot be created is just wrong as it aborts N existing sessions and frustrates N existing users for no good reason. When the key process crashes, the box reboots and it can take minutes to get back to where things were.

Handling error conditions is hard. I won't argue for doing it in generic case as it's been discussed to death and there don't seem to be an agreement on what the right approach should be. However in my case I believe that handling all error conditions is the only way to go.

Thanks for letting me vent.

> If you really need failable construction, investigate GInitable, which has a
> failable pseudo-constructor. g_object_new() still can't fail, but it is
> considered to be an error to use a GInitable without first calling
> g_initable_init() on it, and that can fail; if it does, the only valid thing to
> do with that object is to unref it. GAsyncInitable is similar, but its
> pseudo-constructor is asynchronous.

Thanks for the pointers.
I will have to look further. Given the fact that my code is layered on top of GStreamer code (which sits on top of GLib), implementing this may or may not be easy.

I appreciate the discussion.
Thank you very much!

(In reply to comment #11)
> The only sensible thing to do is to allow the session to be created and then
> see if enough memory exists to support it at runtime. Crashing the process just
> because N+1th session cannot be created is just wrong as it aborts N existing
> sessions and frustrates N existing users for no good reason. When the key
> process crashes, the box reboots and it can take minutes to get back to where
> things were.
> 
> Handling error conditions is hard. I won't argue for doing it in generic case
> as it's been discussed to death and there don't seem to be an agreement on what
> the right approach should be. However in my case I believe that handling all
> error conditions is the only way to go.

Somewhat off-topic but if I was faced with constraints like this, I'd consider splitting my program into two processes: a master process to supervise and orchestrate the work - and a worker process (child of the master process) doing the work. I'd then arrange things (using e.g. /proc/<pid>/oom_adj or similar) such that the kernel's OOM killer would only pick the worker process when running out of physical memory.

With this setup, you simply let the master process watch the child process and, for example, start a simpler (=less memory consuming) worker in the event the worker process is nuked by the OOM killer. If I need the two processes to communicate I'd use something like peer-to-peer D-Bus and gdbus-codegen(1) - then it's almost as easy as if the code ran in the same process.

The point here is that just because GLib aborts on OOM, it does not follow that code using GLib cannot be made to work on systems with limited memory and no overcomit (=no swap).

> Somewhat off-topic but if I was faced with constraints like this, I'd consider
> splitting my program into two processes: a master process to supervise and
> orchestrate the work - and a worker process (child of the master process) doing
> the work. I'd then arrange things (using e.g. /proc/<pid>/oom_adj or similar)
> such that the kernel's OOM killer would only pick the worker process when
> running out of physical memory.

Thanks for ideas.
This sound like a possible way to go, although there are complications in that the code I am sitting on has complex dependencies and some of them I simply do not control. I am not sure how well some of the components that I use will handle multi-instancing and sharing cross process, but it's certainly worth looking into.

A quick question if you don't mind. Is there any supported way to configure glib to not abort g_malloc in case of memory shortage? I know I can go and just change it to do what I want because that part of the code I do control and can modify at will, but I prefer not to if there is a chance.

Thanks!

(In reply to comment #13)
> A quick question if you don't mind. Is there any supported way to configure
> glib to not abort g_malloc in case of memory shortage? I know I can go and just
> change it to do what I want because that part of the code I do control and can
> modify at will, but I prefer not to if there is a chance.

It's generally not possible to return NULL instead of calling abort() here since code using g_malloc() doesn't check for NULL .. so if you return NULL it is at best a SEGFAULT and at worst a buffer overflow. You could sit and spin (or sleep) until memory becomes available, but, really, that's not a good idea as it's a recipe for deadlocks and other disasters.

> It's generally not possible to return NULL instead of calling abort() here
> since code using g_malloc() doesn't check for NULL .. so if you return NULL it
> is at best a SEGFAULT and at worst a buffer overflow. You could sit and spin
> (or sleep) until memory becomes available, but, really, that's not a good idea
> as it's a recipe for deadlocks and other disasters.

OK, that's what I was afraid the answer was going to be.
Thanks for your help!

Review of attachment 198205 [details] [review]:

Looks good