GNOME Bugzilla – Bug 745024
Fix chess crash on wrong clock type in chess pgn
Last modified: 2015-03-24 15:18:27 UTC
If I modify the chess pgn and change to an invalid clock type, chess crashes with: ERROR:chess-clock.c:209:clock_type_string_to_enum: code should not be reached Aborted (core dumped) This behaviour needs to be changed and in no case should we crash on that assert. For an invalid clock, we would want to set the timer to 'no limit'
Created attachment 297686 [details] [review] Fix crash on invalid clock type in pgn
Review of attachment 297686 [details] [review]: I would probably handle this internally to ChessPGN so the rest of the code doesn't have to think about it. ::: lib/chess-clock.vala @@ +16,3 @@ FISCHER, + BRONSTEIN, + INVALID; Hm, how about instead of adding this enum type, you handle this internally in ClockType.string_to_enum() instead. There you could return ClockType.SIMPLE and print a warning, and we would not need any changes in gnome-chess.vala. Then you don't have to get rid of the assertion in ClockType.to_string(). @@ +29,3 @@ return "bronstein"; default: + return "invalid"; This should have been case INVALID, since if the enum value is something else we don't want to return INVALID. (But we don't need to change this function at all if we don't add the INVALID enum value.) ::: src/gnome-chess.vala @@ +475,3 @@ + if (clock_type == ClockType.INVALID) + { + // Print a warning to tell the user that something went wrong So, this is an example of a useless comment. I can read the next line of code. :p But instead of removing it, why don't you mark the string for translation and change it into a translator comment instead.
My concern is that this may be a problem for other PGN tags as well. I would have assumed that the ChessPGN class would take care of sanitizing PGN to make sure all the values are valid.
Will check those too. Thanks for pointing it out!
Created attachment 299337 [details] [review] Fix crash on invalid clock type in pgn * The change in gnome-chess.vala is to avoid printing the warning twice.
Review of attachment 299337 [details] [review]: (In reply to Sahil Sareen from comment #5) > * The change in gnome-chess.vala is to avoid printing the warning twice. Ah, OK, that had me confused. I think a better way to handle this would be to explicitly set the value to simple (hopefully you can just do clock_type = "simple") when printing the warning.
(In reply to Michael Catanzaro from comment #6) > Review of attachment 299337 [details] [review] [review]: > > (In reply to Sahil Sareen from comment #5) > > * The change in gnome-chess.vala is to avoid printing the warning twice. > > Ah, OK, that had me confused. I think a better way to handle this would be > to explicitly set the value to simple (hopefully you can just do clock_type > = "simple") when printing the warning. Hmm, I'm not sure if overwriting the pgn file OR Writing to the pgn tag in a get() is a good idea. Using pgn_game.clock_type would hit the clock_type.get() and try to parse the pgn tag, I could change the pgn tag hash-value in there to "simple" if it was invalid[1] but it doesn't feel right to "set" the pgn tag in a get() function. Did you mean to say something different? [1] tags["X-GNOME-ClockType"]="simple"
Hm, you're right. Maybe it'd be easiest to just not print any warning then. I think there really needs to be no difference in behavior between if (pgn_game.clock_type != null) clock_type = ClockType.string_to_enum (pgn_game.clock_type); and var clock_type_pgn = pgn_game.clock_type; if (clock_type_pgn != null) clock_type = ClockType.string_to_enum (clock_type_pgn); I mean, if those are different, we need to stop and rethink. :p
Created attachment 299383 [details] [review] Fix crash on invalid clock type in pgn (In reply to Michael Catanzaro from comment #8) > Hm, you're right. Maybe it'd be easiest to just not print any warning then. I like the warning though, would be confusing to players otherwise. I came up with something different to handle this. > I think there really needs to be no difference in behavior between > > if (pgn_game.clock_type != null) > clock_type = ClockType.string_to_enum (pgn_game.clock_type); > > and > > var clock_type_pgn = pgn_game.clock_type; > if (clock_type_pgn != null) > clock_type = ClockType.string_to_enum (clock_type_pgn); > > I mean, if those are different, we need to stop and rethink. :p I moved the {check-and-fix + warning} to a helper function that gets called when you parse a pgn game file. So the HashMap of pgn tags you maintain is a sane mapping. - This doesn't overwrite the pgn. (Unless the player saves to the same game file) - This can be easily extended to other pgn tags. - The different behaviour problem in gnome-chess.vala goes away. The only possible downside(?) is if you save the game, the game would be saved with ClockType = "simple". This seems to be logically okay to me because you are essentially playing on a simple clock. Do you feel this is of any concern for players overwriting to their pgn when "Save"ing the game?
Review of attachment 299383 [details] [review]: > Do you feel this is of any concern for players overwriting to their pgn when > "Save"ing the game? Nope, because the player has continued to play with the Simple clock type -- the only sane thing to do is switch the clock type to simple! But now I am confused. What is the behavioral difference from your original patch (besides the double-warning)? Surely just changing the PGNGame object does not trigger a save automatically? ::: lib/chess-pgn.vala @@ +236,3 @@ { public List<PGNGame> games; + private PGNGame game; The PGN object contains a list of PGNGames that the user really ought to be able to choose from when saving/loading. This only works for the last game loaded, I guess.
Created attachment 299408 [details] [review] Fix crash on invalid clock type in pgn (In reply to Michael Catanzaro from comment #10) > Review of attachment 299383 [details] [review] [review]: > > > Do you feel this is of any concern for players overwriting to their pgn when > > "Save"ing the game? > > Nope, because the player has continued to play with the Simple clock type -- > the only sane thing to do is switch the clock type to simple! > > But now I am confused. What is the behavioral difference from your original > patch (besides the double-warning)? Surely just changing the PGNGame object > does not trigger a save automatically? Sorry for the confusion, Both the patches are no different in behaviour, Even the previous one didn't print a double warning. Read on for the difference at the bottom. > > ::: lib/chess-pgn.vala > @@ +236,3 @@ > { > public List<PGNGame> games; > + private PGNGame game; > > The PGN object contains a list of PGNGames that the user really ought to be > able to choose from when saving/loading. This only works for the last game > loaded, I guess. All those sit in List<PGNGame> games. game was just to update one of those, but I realised it didn't make sense to have it as a class variable so reverted back to a local function variable. The code flow for loading a game is like this: 1. load_game() in src/gnome-chess.vala asks to load the pgn game. 2. lib/chess-pgn has a PGN.from_file() which is used. 3. ^Basically reads the pgn file into one big string and calls PGN.from_string which has that local "game" I have referred to before. 4. Once the tag and corresponding value is parsed, this function tries to update PGNGame.HashTable <== tags(key,value) in here I just added a wrapper over this insert function to do some sanity checks and put the correct "value" in that HashTable for the "ClockType" key. The only difference from the first patch is that: Say, the pgn file had ... [X-GNOME-ClockType "asdjkvhsimple"] ... In patch#1 we parse the game file and updated the HashTable as is, i.e. tags["ClockType"] would be "asdjkvhsimple" and we return the right "value" in the get() For patch#2 and #3 we do the sanity checks in the wrapper and keep only sane values in HashTable so get() doesn't have to worry about it. Now both patch#2 and #3 do work exactly the same in behaviour and patch#1 and patch#0 are obsolete. ---- patch#1:https://bugzilla.gnome.org/attachment.cgi?id=299337 patch#2:https://bugzilla.gnome.org/attachment.cgi?id=299383 patch#3:This one.
Review of attachment 299408 [details] [review]: OK! It'd be good to add a FIXME to check some of the other tags in that function as well.
Pushed to master. To ssh://ssareen@git.gnome.org/git/gnome-chess 337f14a..e9d862c master -> master Moving on to the other pgn tags now (BUG746218)