Stockfish 1.6.2 and 1.6.2s (DC) for Mac OS X 10.6

[mcostalba]

Yea, indeed! Without the "moves" it works.

But why is Luis build working?

I have tried by terminal window this morning and it worked either.

I am not able to test now the last sequence you posted, I'll do for sure when I come back home.

Could ypu please write a bit your configuration:

- Release version number

- Compiler name and version

- OS version

Thanks
Marco

[UncombedCoconut]

probably by sheer luck. The effect of a bug that causes memory misuse depends on how the memory is laid out and what values had been in memory.

It's a trivial change to ignore an empty "moves" list. (Note: if you can't reproduce the crash, you can at least reproduce a problem with this input if assertions are turned on.)

Code: Select all

--- src/uci.cpp	2009-12-29 14:18:02.000000000 -0800
+++ src_new/uci.cpp	2010-01-06 12:34:40.988499682 -0800
@@ -205,9 +205,8 @@
         {
             Move move;
             StateInfo st;
-            while (!uip.eof())
+            while (uip >> token)
             {
-                uip >> token;
                 move = move_from_string(RootPosition, token);
                 RootPosition.do_move(move, st);
                 if (RootPosition.rule_50_counter() == 0)

[schlucke]

Stockfish 1.6.2 64bit
gcc version 4.2.1 (Apple Inc. build 5646) (dot 1)
System Version: Mac OS X 10.6.2 (10C540)

Doesn't matter if I compile with -O3 or -fast.

Holger

[zullil]

I compiled with icpc:

icpc (ICC) 11.1 20091012
Copyright (C) 1985-2009 Intel Corporation. All rights reserved.

[sockmonkey]

probably by sheer luck. The effect of a bug that causes memory misuse depends on how the memory is laid out and what values had been in memory.

It's a trivial change to ignore an empty "moves" list. (Note: if you can't reproduce the crash, you can at least reproduce a problem with this input if assertions are turned on.)

Code: Select all

--- src/uci.cpp	2009-12-29 14:18:02.000000000 -0800
+++ src_new/uci.cpp	2010-01-06 12:34:40.988499682 -0800
@@ -205,9 +205,8 @@
         {
             Move move;
             StateInfo st;
-            while (!uip.eof())
+            while (uip >> token)
             {
-                uip >> token;
                 move = move_from_string(RootPosition, token);
                 RootPosition.do_move(move, st);
                 if (RootPosition.rule_50_counter() == 0)

With these changes, my self-compiled (gcc) Stockfishes are much more stable under ExaChess Pro.

Thanks a lot -
Jeremy

[schlucke]

OK, you are lucky! I lost my access to ICC at work

But it seems to be a little bug in SF.

Holger

[UncombedCoconut]

Yea, indeed! Without the "moves" it works.

But why is Luis build working?

I have tried by terminal window this morning and it worked either.

I am not able to test now the last sequence you posted, I'll do for sure when I come back home.

Could ypu please write a bit your configuration:

- Release version number

- Compiler name and version

- OS version

Thanks
Marco

I think these details only matter if istringstream::eof() behaves differently on these C++ libraries. Also, I think the space after "moves" is necessary to cause a crash. (I'm not sure.)

Here is the boring timeline from input to crash in my GCC build crashes.

We parse the position line, see "moves" not followed by end-of-line, and try to grab another token. There isn't one, so the token is still "moves". We parse that (as a move from "mo" to "ve") and get garbage. But there's no sanity check in square_from_string() and we bit-pack the garbage, so it ends up looking like the move g8f7. We execute, moving the black king on top of a black pawn. The resulting position has no black king. During search we search for moves which check the black king, which we "find" on SQ_NONE. This causes out-of-bounds array accesses during bitboard magic, and kaboom.

[mcostalba]

It's a trivial change to ignore an empty "moves" list.

Thanks Justin !

Another good shot from you

I have extended the patch to the whole uci.cpp file because I understood it needed care

The following is the official patch that has been applied:

Code: Select all

Date: Thu, 7 Jan 2010 11:59:32 +0100
Subject: [PATCH] Fix 'position ..... moves ' parsing bug

If after 'moves' there is a space then we crash.

The problem is that operator>>() trims whitespaces so that
after 'moves' has been extract we are still not at eof()
but remaining string contains only spaces. So that the next
extarction operation uip >> token ends up with unchanged token
value that remains 'moves', this garbage value is then feeded
to RootPosition.do_move() through move_from_string() that does
not detect the invalid move value leading to a crash.

This bug is triggered by Shredder 12 interface under Mac that
puts a space after 'moves' without any actual move list.

Bug fixed by Justin Blanchard

After reviewing UCI parsing code I spotted other possible weak
points due to the fact that we don't test if the last extract
operation has been succesful. So I have extended Justing patch
to fix the remaining possible holes in uci.cpp

No functional change.

---
 src/uci.cpp |   46 +++++++++++++++++-----------------------------
 1 files changed, 17 insertions(+), 29 deletions(-)

diff --git a/src/uci.cpp b/src/uci.cpp
index c4c1a03..f7ba206 100644
--- a/src/uci.cpp
+++ b/src/uci.cpp
@@ -107,7 +107,8 @@ namespace {
     UCIInputParser uip(command);
     string token;
 
-    uip >> token; // operator>>() skips any whitespace
+    if (!(uip >> token)) // operator>>() skips any whitespace
+        return true;
 
     if (token == "quit")
         return false;
@@ -159,14 +160,8 @@ namespace {
     else if (token == "perft")
         perft(uip);
     else
-    {
         cout << "Unknown command: " << command << endl;
-        while (!uip.eof())
-        {
-            uip >> token;
-            cout << token << endl;
-        }
-    }
+
     return true;
   }
 
@@ -181,33 +176,33 @@ namespace {
 
     string token;
 
-    uip >> token; // operator>>() skips any whitespace
+    if (!(uip >> token)) // operator>>() skips any whitespace
+        return;
 
     if (token == "startpos")
         RootPosition.from_fen(StartPosition);
     else if (token == "fen")
     {
         string fen;
-        while (token != "moves" && !uip.eof())
+        while (uip >> token && token != "moves")
         {
-            uip >> token;
             fen += token;
             fen += ' ';
         }
         RootPosition.from_fen(fen);
     }
 
-    if (!uip.eof())
+    if (uip.good())
     {
         if (token != "moves")
           uip >> token;
+
         if (token == "moves")
         {
             Move move;
             StateInfo st;
-            while (!uip.eof())
+            while (uip >> token)
             {
-                uip >> token;
                 move = move_from_string(RootPosition, token);
                 RootPosition.do_move(move, st);
                 if (RootPosition.rule_50_counter() == 0)
@@ -231,18 +226,14 @@ namespace {
 
     string token, name;
 
-    uip >> token;
-    if (token == "name")
-    {
-        uip >> name;
-        while (!uip.eof())
-        {
-            uip >> token;
-            if (token == "value")
-                break;
+    if (!(uip >> token)) // operator>>() skips any whitespace
+        return;
 
+    if (token == "name" && uip >> name)
+    {
+        while (uip >> token && token != "value")
             name += (" " + token);
-        }
+
         if (token == "value")
         {
             // Reads until end of line and left trim white space
@@ -276,10 +267,8 @@ namespace {
 
     searchMoves[0] = MOVE_NONE;
 
-    while (!uip.eof())
+    while (uip >> token)
     {
-        uip >> token;
-
         if (token == "infinite")
             infinite = true;
         else if (token == "ponder")
@@ -327,10 +316,9 @@ namespace {
     int depth, tm, n;
     Position pos = RootPosition;
 
-    if (uip.eof())
+    if (!(uip >> depth))
         return;
 
-    uip >> depth;
     tm = get_system_time();
 
     n = perft(pos, depth * OnePly);
-- 
1.6.4.msysgit.0