Fix for Chessbox bug FS#10363

Chessbox was overflowing GameList[240] causing the board to flip + crash

GameCnt changed to unsigned char which allows the array to roll over
to 0 after 255

define MAX_GAME_CNT 256 and GameList[MAX_GAME_CNT] along with 1 byte GameCnt
should fix this issue

dbg save routine left in for now to help identify any other problems

Added bounds checking to prevent second bug found when loading .pgn files

Change-Id: I2b615c8ecbed4368724412f80ce07346f3cf30a7

4 files changed, 164 insertions, 34 deletions

diff --git a/apps/plugins/chessbox/chessbox.c b/apps/plugins/chessbox/chessbox.c
index 86ca5a355e..7d42b72c1b 100644
--- a/apps/plugins/chessbox/chessbox.c
+++ b/apps/plugins/chessbox/chessbox.c
@@ -26,8 +26,7 @@
 #if (MEMORYSIZE > 8) /* Lowmem doesn't have playback in chessbox */
 #define HAVE_PLAYBACK_CONTROL
 #endif
-
-
+/*#define CHESSBOX_SAVE_FILE_DBG  PLUGIN_GAMES_DATA_DIR "/chessbox_dbg.save"*/
 #ifdef HAVE_PLAYBACK_CONTROL
 #include "lib/playback_control.h"
 #endif
@@ -265,12 +264,117 @@ static void cb_levelup ( void ) {
     rb->splash ( HZ/2 , level_string[Level-1] );
 };
 
+#ifdef CHESSBOX_SAVE_FILE_DBG
+/* Save a debug file with names, variables, and sizes */
+static void cb_saveposition_dbg ( void )
+{
+    int fd;
+    short sq,i,c;
+    unsigned short temp;
+    char buf[32]="\0";
+    int ch_ct = 0;
+
+    rb->splash ( 0 , "Saving debug" );
+    fd = rb->open(CHESSBOX_SAVE_FILE_DBG, O_WRONLY|O_CREAT, 0666);
+    ch_ct = rb->snprintf(buf,31,"computer = %d, %d bytes\n",computer+1,
+                                                        sizeof(computer));
+    rb->write(fd, buf, ch_ct);
+    ch_ct = rb->snprintf(buf,31,"opponent = %d, %d bytes\n",opponent+1,
+                                                        sizeof(opponent));
+    rb->write(fd, buf, ch_ct);
+    ch_ct = rb->snprintf(buf,31,"Game50 = %d, %d bytes\n",Game50,
+                                                      sizeof(Game50));
+    rb->write(fd, buf, ch_ct);
+    ch_ct = rb->snprintf(buf,31,"CastldWht = %d, %d bytes\n",castld[white],
+                                                      sizeof(castld[white]));
+    rb->write(fd, buf, ch_ct);
+    ch_ct = rb->snprintf(buf,31,"CastldBlk = %d, %d bytes\n",castld[black],
+                                                      sizeof(castld[black]));
+    rb->write(fd, buf, ch_ct);
+    ch_ct = rb->snprintf(buf,31,"KngMovedWht = %d, %d bytes\n",kingmoved[white],
+                                                      sizeof(kingmoved[white]));
+    rb->write(fd, buf, ch_ct);
+    ch_ct = rb->snprintf(buf,31,"KngMovedBlk = %d, %d bytes\n",kingmoved[black],
+                                                      sizeof(kingmoved[black]));
+    rb->write(fd, buf, ch_ct);
+    ch_ct = rb->snprintf(buf,31,"WithBook = %d, %d bytes\n",withbook,
+                                                      sizeof(withbook));
+    rb->write(fd, buf, ch_ct);
+    ch_ct = rb->snprintf(buf,31,"Lvl = %ld, %d bytes\n",Level,
+                                                      sizeof(Level));
+    rb->write(fd, buf, ch_ct);
+    ch_ct = rb->snprintf(buf,31,"TCflag = %d, %d bytes\n",TCflag,
+                                                      sizeof(TCflag));
+    rb->write(fd, buf, ch_ct);
+    ch_ct = rb->snprintf(buf,31,"OpTime = %d, %d bytes\n",OperatorTime,
+                                                      sizeof(OperatorTime));
+    rb->write(fd, buf, ch_ct);
+    ch_ct = rb->snprintf(buf,31,"TmCtlClkWht = %ld, %d bytes\n",
+                    TimeControl.clock[white], sizeof(TimeControl.clock[white]));
+    rb->write(fd, buf, ch_ct);
+    ch_ct = rb->snprintf(buf,31,"TmCtlClkBlk = %ld, %d bytes\n",
+                    TimeControl.clock[black],  sizeof(TimeControl.clock[black]));
+    rb->write(fd, buf, ch_ct);
+    ch_ct = rb->snprintf(buf,31,"TmCtlMovesWht = %d, %d bytes\n",
+                   TimeControl.moves[white],  sizeof(TimeControl.moves[white]));
+    rb->write(fd, buf, ch_ct);
+    ch_ct = rb->snprintf(buf,31,"TmCtlMovesBlk = %d, %d bytes\n",
+                   TimeControl.moves[black],  sizeof(TimeControl.moves[black]));
+    rb->write(fd, buf, ch_ct);
+    for (sq = 0; sq < 64; sq++) {
+        if (color[sq] == neutral) c = 0; else c = color[sq]+1;
+        temp = 256*board[sq] + c ;
+        ch_ct = rb->snprintf(buf,31,"sq %02d = %d, %d bytes\n",sq, temp,
+                                                      sizeof(temp));
+        rb->write(fd, buf, ch_ct);
+    }
+    for (i = 0; i <= GameCnt; i++) {
+        ch_ct = rb->snprintf(buf,31,"GameCt %d, %d bytes\n",i,
+                                                      sizeof(GameCnt));
+        rb->write(fd, buf, ch_ct);
+        if (GameList[i].color == neutral)
+        {
+            c = 0;
+            ch_ct = rb->snprintf(buf,31,"color = %d, %d bytes\n",c,
+                                                            sizeof(c));
+            rb->write(fd, buf, ch_ct);
+        }
+        else
+            c = GameList[i].color + 1;
+        ch_ct = rb->snprintf(buf,31,"gmove = %d, %d bytes\n",GameList[i].gmove,
+                                                      sizeof(GameList[i].gmove));
+        rb->write(fd, buf, ch_ct);
+        ch_ct = rb->snprintf(buf,31,"score = %d, %d bytes\n",GameList[i].score,
+                                                      sizeof(GameList[i].score));
+        rb->write(fd, buf, ch_ct);
+        ch_ct = rb->snprintf(buf,31,"depth = %d, %d bytes\n",GameList[i].depth,
+                                                      sizeof(GameList[i].depth));
+        rb->write(fd, buf, ch_ct);
+        ch_ct = rb->snprintf(buf,31,"nodes = %ld, %d bytes\n",GameList[i].nodes,
+                                                      sizeof(GameList[i].nodes));
+        rb->write(fd, buf, ch_ct);
+        ch_ct = rb->snprintf(buf,31,"time = %d, %d bytes\n",GameList[i].time,
+                                                      sizeof(GameList[i].time));
+        rb->write(fd, buf, ch_ct);
+        ch_ct = rb->snprintf(buf,31,"piece = %d, %d bytes\n",GameList[i].piece,
+                                                     sizeof(GameList[i].piece));
+        rb->write(fd, buf, ch_ct);
+        ch_ct = rb->snprintf(buf,31,"color = %d, %d bytes\n",c,sizeof(c));
+        rb->write(fd, buf, ch_ct);
+    }
+    rb->close(fd);
+
+}
+#endif
+
 /* ---- Save current position ---- */
 static void cb_saveposition ( void ) {
     int fd;
     short sq,i,c;
     unsigned short temp;
-
+#ifdef CHESSBOX_SAVE_FILE_DBG
+    cb_saveposition_dbg();
+#endif
     rb->splash ( 0 , "Saving position" );
 
     fd = rb->open(SAVE_FILE, O_WRONLY|O_CREAT, 0666);
@@ -356,7 +460,7 @@ static void cb_restoreposition ( void ) {
             else
                 --color[sq];
         }
-        GameCnt = -1;
+        GameCnt = MAX_GAME_CNT - 1; /*uchar rollsover to 0 after 255*/
         while (rb->read(fd, &(GameList[++GameCnt].gmove),
                         sizeof(GameList[GameCnt].gmove)) > 0) {
             rb->read(fd, &(GameList[GameCnt].score),
diff --git a/apps/plugins/chessbox/chessbox_pgn.c b/apps/plugins/chessbox/chessbox_pgn.c
index 98d9e29431..f5f19e2688 100644
--- a/apps/plugins/chessbox/chessbox_pgn.c
+++ b/apps/plugins/chessbox/chessbox_pgn.c
@@ -159,9 +159,10 @@ static char pgn_from_piece(unsigned short piece, unsigned short color){
     return pgn_piece;
 }
 
-static void pgn_to_coords(struct pgn_ply_node* ply){
+static bool pgn_to_coords(struct pgn_ply_node* ply){
+    bool success = true;
     unsigned short str_length = rb->strlen(ply->pgn_text);
-    char str[10];
+    char str[9];
     rb->strcpy(str,ply->pgn_text);
     ply->column_from = 0xFF;
     ply->row_from = 0xFF;
@@ -172,6 +173,7 @@ static void pgn_to_coords(struct pgn_ply_node* ply){
     ply->enpassant = false;
     ply->castle = false;
     ply->promotion = false;
+
     unsigned short i, j, piece;
     bool found = false;
 
@@ -342,16 +344,25 @@ static void pgn_to_coords(struct pgn_ply_node* ply){
     /* leave a very complete log of the parsing of the game while it gets stable */
     for (i=0;i<8;i++){
         for (j=0;j<8;j++) {
+
             rb->fdprintf(loghandler,"%c",pgn_from_piece(board[locn[7-i][j]],color[locn[7-i][j]]));
         }
         rb->fdprintf(loghandler,"\n");
     }
-
+    /* check bounds of row and columns should be 0-7 bad .pgn returns 0xFF */
+    if ((ply->row_to | ply->column_to | ply->row_from | ply->column_from) < 8)
+    {
     /* update the board */
-    board[locn[ply->row_to][ply->column_to]] = board[locn[ply->row_from][ply->column_from]];
-    color[locn[ply->row_to][ply->column_to]] = color[locn[ply->row_from][ply->column_from]];
-    board[locn[ply->row_from][ply->column_from]] = no_piece;
-    color[locn[ply->row_from][ply->column_from]] = neutral;
+        board[locn[ply->row_to][ply->column_to]] =
+                                   board[locn[ply->row_from][ply->column_from]];
+        color[locn[ply->row_to][ply->column_to]] =
+                                    color[locn[ply->row_from][ply->column_from]];
+        board[locn[ply->row_from][ply->column_from]] = no_piece;
+        color[locn[ply->row_from][ply->column_from]] = neutral;
+    }
+    else
+        success = false; /*ERROR*/
+    return success;
 }
 
 static void coords_to_pgn(struct pgn_ply_node* ply){
@@ -657,9 +668,10 @@ void pgn_parse_game(const char* filename,
                     struct pgn_game_node* selected_game){
     struct pgn_ply_node size_ply, *first_ply = NULL;
     struct pgn_ply_node *temp_ply = NULL, *curr_node = NULL;
+    bool success = true;
     int fhandler, i;
     char line_buffer[128];
-    char token_buffer[10];
+    char token_buffer[9];
     unsigned short pos;
     unsigned short curr_player = white;
 
@@ -685,24 +697,35 @@ void pgn_parse_game(const char* filename,
                 || (token_buffer[0] >= 'a' && token_buffer[0] <= 'z')
                 || (token_buffer[0] == '0' && token_buffer[2] != '1')){
                 temp_ply = (struct pgn_ply_node *)pl_malloc(sizeof size_ply);
-                temp_ply->player = curr_player;
-                curr_player = (curr_player==white)?black:white;
-                rb->strcpy(temp_ply->pgn_text, token_buffer);
-                pgn_to_coords(temp_ply);
-                temp_ply->prev_node = NULL;
-                temp_ply->next_node = NULL;
-                if (first_ply == NULL) {
-                    first_ply = curr_node = temp_ply;
-                } else {
-                    curr_node->next_node = temp_ply;
-                    temp_ply->prev_node = curr_node;
-                    curr_node = temp_ply;
-                }
-                rb->fdprintf(loghandler,
-                    "player: %u; pgn: %s; from: %u,%u; to: %u,%u; taken: %u.\n",
+                /* Null pointer can be returned from pl_malloc check for this */
+                if (temp_ply)
+                {
+                    temp_ply->player = curr_player;
+                    curr_player = (curr_player==white)?black:white;
+                    rb->strcpy(temp_ply->pgn_text, token_buffer);
+                    success = pgn_to_coords(temp_ply);
+                    temp_ply->prev_node = NULL;
+                    temp_ply->next_node = NULL;
+                    if (first_ply == NULL && success) {
+                        first_ply = curr_node = temp_ply;
+                    } else if (success){
+                        curr_node->next_node = temp_ply;
+                        temp_ply->prev_node = curr_node;
+                        curr_node = temp_ply;
+                    } else{
+                       /* bad .pgn break loop and notify user */
+                       first_ply = NULL;
+                        break;
+                    }
+
+                    rb->fdprintf(loghandler,
+                        "player: %u; pgn: %s; from: %u,%u; to: %u,%u; taken: %u.\n",
                              temp_ply->player, temp_ply->pgn_text, temp_ply->row_from,
                              temp_ply->column_from, temp_ply->row_to,
                              temp_ply->column_to, temp_ply->taken_piece);
+                }
+                else
+                    first_ply = NULL;
             }
         }
     }
@@ -719,6 +742,7 @@ void pgn_parse_game(const char* filename,
         curr_node->next_node = temp_ply;
     }
     selected_game->first_ply = first_ply;
+
     rb->close(fhandler);
 }
 
diff --git a/apps/plugins/chessbox/gnuchess.c b/apps/plugins/chessbox/gnuchess.c
index 5e67df4f39..4b21cd785c 100644
--- a/apps/plugins/chessbox/gnuchess.c
+++ b/apps/plugins/chessbox/gnuchess.c
@@ -103,8 +103,9 @@ short INCscore;
 short HasPawn[2],HasKnight[2],HasBishop[2],HasRook[2],HasQueen[2];
 short ChkFlag[maxdepth],CptrFlag[maxdepth],PawnThreat[maxdepth];
 short Pscore[maxdepth],Tscore[maxdepth],Threat[maxdepth];
-struct GameRec GameList[240];
-short GameCnt,Game50,epsquare,lpost,rcptr,contempt;
+struct GameRec GameList[MAX_GAME_CNT];
+unsigned char GameCnt; /*Bug fix now rolls over instead of overflow*/
+short Game50,epsquare,lpost,rcptr,contempt;
 short MaxSearchDepth,Xscore;
 struct TimeControlRec TimeControl;
 short TCflag,TCmoves,TCminutes,OperatorTime;
@@ -1132,7 +1133,7 @@ static short i,alpha,beta,score,tempb,tempc,tempsf,tempst,xside,rpt;
       if (--TimeControl.moves[side] == 0) SetTimeControl();
     }
   if ((root->flags & draw) && bothsides) quit = true;
-  if (GameCnt > 238) quit = true;
+  if (GameCnt > MAX_GAME_CNT - 2) quit = true;
   player = xside;
   Sdepth = 0;
   return(0);
@@ -2319,7 +2320,7 @@ void NewGame()  {
   xwndw = 90;
   MaxSearchDepth = 29;
   contempt = 0;
-  GameCnt = -1; Game50 = 0;
+  GameCnt = MAX_GAME_CNT - 1; Game50 = 0;
   Zwmtl = Zbmtl = 0;
   Developed[white] = Developed[black] = false;
   castld[white] = castld[black] = false;
diff --git a/apps/plugins/chessbox/gnuchess.h b/apps/plugins/chessbox/gnuchess.h
index b7a3a309f8..f52e1b1a0a 100644
--- a/apps/plugins/chessbox/gnuchess.h
+++ b/apps/plugins/chessbox/gnuchess.h
@@ -1,7 +1,7 @@
 
 #ifndef _GNUCHESS_H_
 #define _GNUCHESS_H_
-
+#define MAX_GAME_CNT 256
 #define neutral 2
 #define white 0
 #define black 1
@@ -39,9 +39,10 @@ extern bool withbook;
 extern long Level;
 extern short TCflag,TCmoves,TCminutes;
 extern short timeout;
-extern short GameCnt,Game50,castld[2],kingmoved[2],OperatorTime;
+extern unsigned char GameCnt; /* Bug fix rolls over at 255 instead of overflow */
+extern short Game50,castld[2],kingmoved[2],OperatorTime;
 extern struct TimeControlRec TimeControl;
-extern struct GameRec GameList[240];
+extern struct GameRec GameList[MAX_GAME_CNT];
 
 /* ---- The beginning of a GNUChess v2 APIfication ---- */
 void SetTimeControl(void);