Unknown brush settings cause crash on assert

[arcusfelis]

Hi, I'm trying to use the lib with Anti or "dirty" brushes in Gimp. Gimp loads brushes from JSON. With some brushes there are unknown settings.

So, it triggers

assert (id >= 0 && id < MYPAINT_BRUSH_SETTINGS_COUNT); 

in mypaint_brush_set_base_value called from update_settings_from_json_object.

My solution is

    // Set settings                                                                  
    json_object *settings = NULL;                                                    
    if (! obj_get(self->brush_json, "settings", &settings)) {                        
        fprintf(stderr, "Error: No 'settings' field for brush\n");                   
        return FALSE;                                                                
    }                                                                                

    json_object_object_foreach(settings, setting_name, setting_obj) {                

        MyPaintBrushSetting setting_id = mypaint_brush_setting_from_cname(setting_name);

       //  NEW CHECK                                                   
        if (!(setting_id >= 0 && setting_id < MYPAINT_BRUSH_SETTINGS_COUNT)) {       
            fprintf(stderr, "Error: Unknown setting_id: %d for setting: %s\n",       
                    setting_id, setting_name);                                       
//          return FALSE;                                                            
            continue;                                                                
        }                                                                            

        if (!json_object_is_type(setting_obj, json_type_object)) {                   
            fprintf(stderr, "Error: Wrong type for setting: %s\n", setting_name);    
            return FALSE;                                                            
        } 

It just ignores some settings but still loads the brush. Alternative behaviour is to return FALSE, but then the brush becomes useless. My code returns:

Error: Unknown setting_id: -1 for setting: offset_angle
Error: Unknown setting_id: -1 for setting: offset_angle_2
Error: Unknown setting_id: -1 for setting: offset_x
Error: Unknown setting_id: -1 for setting: offset_y
Error: Unknown setting_id: -1 for setting: offset_angle
Error: Unknown setting_id: -1 for setting: offset_angle_2
Error: Unknown setting_id: -1 for setting: offset_x
Error: Unknown setting_id: -1 for setting: offset_y
Error: Unknown setting_id: -1 for setting: offset_angle
Error: Unknown setting_id: -1 for setting: offset_angle_2
Error: Unknown setting_id: -1 for setting: offset_x
Error: Unknown setting_id: -1 for setting: offset_y
Error: Unknown setting_id: -1 for setting: offset_angle
Error: Unknown setting_id: -1 for setting: offset_angle_2
Error: Unknown setting_id: -1 for setting: offset_x
Error: Unknown setting_id: -1 for setting: offset_y
Error: Unknown setting_id: -1 for setting: offset_angle
Error: Unknown setting_id: -1 for setting: offset_angle_2
Error: Unknown setting_id: -1 for setting: offset_x
Error: Unknown setting_id: -1 for setting: offset_y
Error: Unknown setting_id: -1 for setting: offset_angle
Error: Unknown setting_id: -1 for setting: offset_angle_2
Error: Unknown setting_id: -1 for setting: offset_x
Error: Unknown setting_id: -1 for setting: offset_y
Error: Unknown setting_id: -1 for setting: offset_angle
Error: Unknown setting_id: -1 for setting: offset_angle_2
Error: Unknown setting_id: -1 for setting: offset_x
Error: Unknown setting_id: -1 for setting: offset_y

So, my question before I've started preparing any pull request, which solution is more fitting there:

• Return FALSE;
• Ignore error;
• Ignore error but print error/set another exit code;
• Lets caller of the function to decide if he want to ignore errors or not;
• Current way (assert).

[briend]

It's be great if these messages could be ignored (maybe printed on stderr). Another related issue is when your MyPaint gui is sending an incorrect number of arguments to stroke_to that libmypaint is expecting. It'd be nice if libmypaint could possibly fail gracefully in the case of too many or too few arguments.

[shakaran]

@jonnor @achadwick BUMP any update for fix this bug or help to @arcusfelis to prepare a quick PR?

This bug is pretty critical for GIMP 2.10 release as it is blocker https://bugzilla.gnome.org/show_bug.cgi?id=767662

[Jehan]

Hi,

I suggest to follow the current behavior for a first patch, i.e. return FALSE please. Otherwise it is inconsistent with the other checks. Also by definition, you should output a Warning, not an error, if you were just ignoring the unknown setting. @arcusfelis So please make a patch which doesn't ignore error and return FALSE and I will merge it.

As a second step, we can discuss an improvement of the whole "degrade gracefully" idea.

[jonnor]

Agreed with @Jehan, return FALSE and provide a warning.

@arcusfelis let us know if you need help in providing a patch / pull request.

[achadwick]

This is worth getting in for 1.3.0.

[achadwick]

Okay, it is simplest for the function to return FALSE and emit an "Error:" (not a "Warning:" because we are deciding to fail the entire brush load). That's all by analogy with the current way of handling "wrong type of thing" or a missing base_value key in the setting object.

So for now that's what we'll do.

I would prefer that the entire body of the json_object_object_foreach() move to a static function that returns whether the current name/value pair was successfully converted to a base value and zero or more mapping points. And to relabel the printf()s as "Warning:"s in that case :)

However, degrading cleanly may need extra API and break ABI. A libmypaint-using program may want to present the user with a message about some settings not being loaded, and that may require mypaint_brush_from_string() to return more than a boolean. If we can do without this use case then everything can be made API and ABI compatible, and maybe we'll do it for 1.3.1. Otherwise it'll have to be new for 1.4 at the earliest, as a new function.

To decide: do users of this function need to know whether all settings loaded cleanly?

[achadwick]

The more I look at this, the more I think that being tolerant on input is the right thing to do. I think it's more correct to return TRUE from mypaint_brush_from_string() if any settings were updated, not all. Test code and cases to play with shortly.

[achadwick]

Okay, that branch (issue74) passes an updated "make check". Let me know if you agree with the new split between warnings (per-setting stuff) and errors (global stuff, including a completely missing settings section).

[Jehan]

Well in GIMP, we'll be happy with no crashes at least. Now I am wondering about the warnings to stderr. As you said yourself it will be a little annoying to not being able to tell the artists through the GUI that brushes were incompletely loaded. Typically they will just think "it's broken" and blame the program (GIMP for instance) instead of thinking they have broken/outdated data.

Of course, a API/ABI break would also need to be taken seriously so this is perfectly acceptable as a first version.

[arcusfelis]

Many brushes from Anti set are still usable even with some ignored settings parameters. But it would be good to somehow identify "broken" brushes.

One way is to set some attribute to the brush that marks it as not properly loaded and display it as red/gray icon in UI (and maybe adding "Error details..." to the icon context menu). It will require changes on gimp side too.

Another way is create a "broken" group of brushes.

But I guess it's a bit too complicated. Because user will not be able to fix the brush (there is no way to edit brushes in Gimp, only in MyPaint). We can ignore the error for now and write to stderr. Later we can introduce some way of getting debug info.

[achadwick]

Fixed up a bunch more segfaults, and added test cases for them. All looking good.