Subject: bug#8254: closed (race condition in dired.c's scmp function)

From: [email-masked] (GNU bug Tracking System)
Date: Thu, 17 Mar 2011 16:57:02 +0000

Your bug report

#8254: race condition in dired.c's scmp function

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
8254: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8254
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: race condition in dired.c's scmp function
From: Paul Eggert <[email-masked]>
To: [email-masked]
Date: Thu, 17 Mar 2011 09:56:16 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

I've committed a patch for this in bzr 103679.

Subject: race condition in dired.c's scmp function
From: Paul Eggert <[email-masked]>
To: [email-masked]
Date: Mon, 14 Mar 2011 23:16:26 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

The following code in the Emacs trunk src/dired.c's scmp function has
undefined behavior:

      while (l
	     && (DOWNCASE ((unsigned char) *s1++)
		 == DOWNCASE ((unsigned char) *s2++)))
	l--;

Because the DOWNCASE macro assigns to the global variables case_temp1
and case_temp2, (DOWNCASE (x) == DOWNCASE (y)) is not valid, as the
assignments can collide and lead to a race condition.

This bug was found by gcc -Wsequence-point (GCC 4.5.2), which warns:

  dired.c:799:7: error: operation on 'case_temp2' may be undefined [-Wsequence-point]
  dired.c:799:7: error: operation on 'case_temp1' may be undefined [-Wsequence-point]

I plan to work around the problem with something like the following
patch.

----

Fix a race condition diagnosed by gcc -Wsequence-point.
The expression (DOWNCASE ((unsigned char) *s1++) ==
DOWNCASE ((unsigned char) *s2++)), found in dired.c's scmp
function, had undefined behavior.
* lisp.h (DOWNCASE_TABLE, UPCASE_TABLE, DOWNCASE, UPPERCASEP):
(NOCASEP, LOWERCASEP, UPCASE, UPCASE1): Move from here ...
* buffer.h: ... to here, because these macros use current_buffer,
and the new implementation with inline functions needs to have
current_buffer in scope now, rather than later when the macros
are used.
(downcase, upcase1): New static inline functions.
(DOWNCASE, UPCASE1): Reimplement using these functions.
This avoids undefined behavior in expressions like
DOWNCASE (x) == DOWNCASE (y), which previously suffered
from race conditions in accessing the global variables
case_temp1 and case_temp2.
* casetab.c (case_temp1, case_temp2): Remove; no longer needed.
* lisp.h (case_temp1, case_temp2): Remove their decls.
* character.h (ASCII_CHAR_P): Move from here ...
* lisp.h: ... to here, so that the inline functions mentioned
above can use them.
=== modified file 'src/buffer.h'
--- src/buffer.h	2011-03-13 22:25:16 +0000
+++ src/buffer.h	2011-03-15 05:52:48 +0000
@@ -1026,4 +1026,47 @@

 #define PER_BUFFER_VALUE(BUFFER, OFFSET) \
       (*(Lisp_Object *)((OFFSET) + (char *) (BUFFER)))
-
+
+/* Current buffer's map from characters to lower-case characters.  */
+
+#define DOWNCASE_TABLE BVAR (current_buffer, downcase_table)
+
+/* Current buffer's map from characters to upper-case characters.  */
+
+#define UPCASE_TABLE BVAR (current_buffer, upcase_table)
+
+/* Downcase a character, or make no change if that cannot be done.  */
+
+static inline EMACS_INT
+downcase (int ch)
+{
+  Lisp_Object down = CHAR_TABLE_REF (DOWNCASE_TABLE, ch);
+  return NATNUMP (down) ? XFASTINT (down) : ch;
+}
+#define DOWNCASE(CH) downcase (CH)
+
+/* 1 if CH is upper case.  */
+
+#define UPPERCASEP(CH) (DOWNCASE (CH) != (CH))
+
+/* 1 if CH is neither upper nor lower case.  */
+
+#define NOCASEP(CH) (UPCASE1 (CH) == (CH))
+
+/* 1 if CH is lower case.  */
+
+#define LOWERCASEP(CH) (!UPPERCASEP (CH) && !NOCASEP(CH))
+
+/* Upcase a character, or make no change if that cannot be done.  */
+
+#define UPCASE(CH) (!UPPERCASEP (CH) ? UPCASE1 (CH) : (CH))
+
+/* Upcase a character known to be not upper case.  */
+
+static inline EMACS_INT
+upcase1 (int ch)
+{
+  Lisp_Object up = CHAR_TABLE_REF (UPCASE_TABLE, ch);
+  return NATNUMP (up) ? XFASTINT (up) : ch;
+}
+#define UPCASE1(CH) upcase1 (CH)

=== modified file 'src/casetab.c'
--- src/casetab.c	2011-02-16 15:02:50 +0000
+++ src/casetab.c	2011-03-15 04:01:35 +0000
@@ -28,11 +28,6 @@
 Lisp_Object Vascii_downcase_table, Vascii_upcase_table;
 Lisp_Object Vascii_canon_table, Vascii_eqv_table;

-/* Used as a temporary in DOWNCASE and other macros in lisp.h.  No
-   need to mark it, since it is used only very temporarily.  */
-int case_temp1;
-Lisp_Object case_temp2;
-
 static void set_canon (Lisp_Object case_table, Lisp_Object range, Lisp_Object elt);
 static void set_identity (Lisp_Object table, Lisp_Object c, Lisp_Object elt);
 static void shuffle (Lisp_Object table, Lisp_Object c, Lisp_Object elt);
@@ -302,4 +297,3 @@
   defsubr (&Sset_case_table);
   defsubr (&Sset_standard_case_table);
 }
-

=== modified file 'src/character.h'
--- src/character.h	2011-03-08 04:37:19 +0000
+++ src/character.h	2011-03-15 05:52:52 +0000
@@ -128,9 +128,6 @@
     XSETCDR ((x), tmp);			\
   } while (0)

-/* Nonzero iff C is an ASCII character.  */
-#define ASCII_CHAR_P(c) ((unsigned) (c) < 0x80)
-
 /* Nonzero iff C is a character of code less than 0x100.  */
 #define SINGLE_BYTE_CHAR_P(c) ((unsigned) (c) < 0x100)

=== modified file 'src/lisp.h'
--- src/lisp.h	2011-03-15 01:32:33 +0000
+++ src/lisp.h	2011-03-15 04:23:51 +0000
@@ -841,6 +841,9 @@

 #endif	/* not __GNUC__ */

+/* Nonzero iff C is an ASCII character.  */
+#define ASCII_CHAR_P(c) ((unsigned) (c) < 0x80)
+
 /* Almost equivalent to Faref (CT, IDX) with optimization for ASCII
    characters.  Do not check validity of CT.  */
 #define CHAR_TABLE_REF(CT, IDX)					\
@@ -2041,50 +2044,6 @@

 #define QUITP (!NILP (Vquit_flag) && NILP (Vinhibit_quit))
 
-/* Variables used locally in the following case handling macros.  */
-extern int case_temp1;
-extern Lisp_Object case_temp2;
-
-/* Current buffer's map from characters to lower-case characters.  */
-
-#define DOWNCASE_TABLE BVAR (current_buffer, downcase_table)
-
-/* Current buffer's map from characters to upper-case characters.  */
-
-#define UPCASE_TABLE BVAR (current_buffer, upcase_table)
-
-/* Downcase a character, or make no change if that cannot be done.  */
-
-#define DOWNCASE(CH)						\
-  ((case_temp1 = (CH),						\
-    case_temp2 = CHAR_TABLE_REF (DOWNCASE_TABLE, case_temp1),	\
-    NATNUMP (case_temp2))					\
-   ? XFASTINT (case_temp2) : case_temp1)
-
-/* 1 if CH is upper case.  */
-
-#define UPPERCASEP(CH) (DOWNCASE (CH) != (CH))
-
-/* 1 if CH is neither upper nor lower case.  */
-
-#define NOCASEP(CH) (UPCASE1 (CH) == (CH))
-
-/* 1 if CH is lower case.  */
-
-#define LOWERCASEP(CH) (!UPPERCASEP (CH) && !NOCASEP(CH))
-
-/* Upcase a character, or make no change if that cannot be done.  */
-
-#define UPCASE(CH) (!UPPERCASEP (CH) ? UPCASE1 (CH) : (CH))
-
-/* Upcase a character known to be not upper case.  */
-
-#define UPCASE1(CH)						\
-  ((case_temp1 = (CH),						\
-    case_temp2 = CHAR_TABLE_REF (UPCASE_TABLE, case_temp1),	\
-    NATNUMP (case_temp2))					\
-   ? XFASTINT (case_temp2) : case_temp1)
-
 extern Lisp_Object Vascii_downcase_table, Vascii_upcase_table;
 extern Lisp_Object Vascii_canon_table, Vascii_eqv_table;