GNOME Bugzilla – Bug 704990
Validate input when opening files
Last modified: 2015-04-21 14:20:54 UTC
Created attachment 250267 [details] Test Golbat If you're stupid and try to open something that's not a PGN, you're likely to crash GNOME Chess. We need better input validation. Test file attached.
Created attachment 293482 [details] [review] Allow loading only pgn files to avoid crashes 1. Would it be a good idea to allow loading only pgn files?[patch] 2. Does this bug mean to address validating the moves as well along with all the initial fields in the pgn file?
Created attachment 293485 [details] Test Golbat #2 > 1. Would it be a good idea to allow loading only pgn files?[patch] No, because then I can still crash Chess just by changing the file extension. :) To fix this bug, look at the backtrace of the crash to see why the crash occurred, then display a warning dialog ("Uh-oh! This doesn't look like a valid chess game log.") instead of crashing. [New LWP 23602] [New LWP 23603] [New LWP 23604] [New LWP 23606] [New LWP 23619] [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". Core was generated by `gnome-chess'. Program terminated with signal SIGSEGV, Segmentation fault.
+ Trace 234482
Thread 1 (Thread 0x7fbe1df97980 (LWP 23602))
Oh, and that trace was taken on 3.14.1. The crash occurred in gnome-chess.vala:448, which is: string[] moves = new string[pgn_game.moves.length ()];
Created attachment 301841 [details] [review] Validate PGN Display a dialog telling the player if he tries to load an invalid PGN file. Made sure that the game_file is set to null to avoid setting the filename in the header bar.
Review of attachment 301841 [details] [review]: Nits: ::: src/gnome-chess.vala @@ +36,2 @@ private Gtk.Dialog? preferences_dialog = null; + private Gtk.Dialog? invalid_pgn_dialog = null; Since it's used only in one function, it doesn't need to be a member variable, correct? @@ +2144,3 @@ } + public void invalid_pgn_draw_dialog () Since you're not mucking with Cairo, I wouldn't include "draw" in the name of the function. Perhaps run_invalid_pgn_dialog()? Also, it can be private, right? @@ +2154,3 @@ + var invalid_pgn_dialog = new Gtk.MessageDialog (window, + Gtk.DialogFlags.MODAL, + Gtk.MessageType.QUESTION, Gtk.MessageType.ERROR (or maybe Gtk.MessageType.WARNING) @@ +2443,3 @@ + pgn_game = new PGNGame (); + game_file = null; + } else { I think we always have opening bracket on the next line in Chess. (Why didn't your script catch this? :-)
Created attachment 301867 [details] [review] Validate PGN
(In reply to Michael Catanzaro from comment #5) > I think we always have opening bracket on the next line in Chess. (Why > didn't your script catch this? :-) The script doesn't check this yet, The reason I didn't put this check in was because I wasn't sure if it applies to all games. Should be straight forward to add though.
Review of attachment 301867 [details] [review]: ::: src/gnome-chess.vala @@ +2145,3 @@ + private void run_invalid_pgn_dialog () + { + Gtk.Dialog? invalid_pgn_dialog = new Gtk.MessageDialog (window, OK, but now it should not be nullable because it should never be null. :) Accepted with that tweak. Actually, we normally use the var keyword whenever possible, so I would use that instead.
Also, all Vala games except Sudoku use the same style. But Sudoku is internally inconsistent, so at some point we'll want to fix that anyway.
Created attachment 302072 [details] [review] Validate PGN Changed to 'var'.
(In reply to Michael Catanzaro from comment #9) > Also, all Vala games except Sudoku use the same style. But Sudoku is > internally inconsistent, so at some point we'll want to fix that anyway. I'll take care of that by tomorrow EOD and then update the script to check brackets.
Fix pushed to master. To ssh://ssareen@git.gnome.org/git/gnome-chess ba128dd..0fbed8d master -> master