New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unknown brush settings cause crash on assert #74

Closed
arcusfelis opened this Issue Oct 26, 2016 · 11 comments

Comments

Projects
None yet
6 participants
@arcusfelis

arcusfelis commented Oct 26, 2016

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

This comment has been minimized.

Show comment
Hide comment
@briend

briend Oct 26, 2016

Contributor

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.

Contributor

briend commented Oct 26, 2016

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

This comment has been minimized.

Show comment
Hide comment
@shakaran

shakaran Nov 8, 2016

Contributor

@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

Contributor

shakaran commented Nov 8, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@Jehan

Jehan Nov 14, 2016

Member

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.

Member

Jehan commented Nov 14, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jonnor

jonnor Nov 14, 2016

Member

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

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

Member

jonnor commented Nov 14, 2016

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

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

@achadwick achadwick added the bug label Nov 29, 2016

@achadwick achadwick added this to the v1.3.0 milestone Nov 29, 2016

@achadwick

This comment has been minimized.

Show comment
Hide comment
@achadwick

achadwick Nov 29, 2016

Member

This is worth getting in for 1.3.0.

Member

achadwick commented Nov 29, 2016

This is worth getting in for 1.3.0.

@achadwick achadwick self-assigned this Nov 29, 2016

@achadwick

This comment has been minimized.

Show comment
Hide comment
@achadwick

achadwick Nov 29, 2016

Member

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?

Member

achadwick commented Nov 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@achadwick

achadwick Nov 29, 2016

Member

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.

Member

achadwick commented Nov 29, 2016

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 added a commit that referenced this issue Nov 29, 2016

achadwick added a commit that referenced this issue Nov 29, 2016

test-brush-persistence: fail if load fails
This expresses the "degrade gracefully" approach discussed in
mypaint/libmypaint#74 by making JSON loading fail if any settings were
recognised and updated correctly.

[ci skip]

achadwick added a commit that referenced this issue Nov 29, 2016

More tolerant brush settings json loader
Factor out the settings foreach loop, and include @arcusfelis's test
to avoid an assertion.

Closes mypaint/libmypaint#74
@achadwick

This comment has been minimized.

Show comment
Hide comment
@achadwick

achadwick Nov 29, 2016

Member

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).

Member

achadwick commented Nov 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@Jehan

Jehan Nov 29, 2016

Member

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.

Member

Jehan commented Nov 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@arcusfelis

arcusfelis Nov 30, 2016

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.

arcusfelis commented Nov 30, 2016

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 added a commit that referenced this issue Dec 1, 2016

json loading: add test cases for pathological data
Add several test cases for bad JSON data.

More followup to mypaint/libmypaint#74.
@achadwick

This comment has been minimized.

Show comment
Hide comment
@achadwick

achadwick Dec 1, 2016

Member

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

Member

achadwick commented Dec 1, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment