all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* Too permissive compare-strings?
@ 2014-06-24 14:03 Dmitry Antipov
  2014-06-25  3:33 ` Stefan Monnier
  0 siblings, 1 reply; 2+ messages in thread
From: Dmitry Antipov @ 2014-06-24 14:03 UTC (permalink / raw)
  To: Emacs development discussions

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]

Recently I tried to utilize the common substring range validation
function validate_subarray in Fcompare_strings (patch is attached).
Surprisingly, I also found that current implementation of this
function silently allows invalid ranges. E.g. this is not an error:

(compare-strings "test" 0 100 "testbed" 0 4)

Although 100 is invalid, this is 't'. Compare it with substring:

(substring "test" 0 100) ==> not "test", but Args out of range: "test", 0, 100

Not too serious problem? Wrong. A lot of existing Lisp code assumes
current semantic of compare-strings and tends to specify range end which is
beyond the end of string. After applying my patch, I was unable to bootstrap,
with a lot of errors from lisp/subr.el and lisp/files.el.

Thoughts?

Dmitry

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: compare_strings.patch --]
[-- Type: text/x-patch; name="compare_strings.patch", Size: 3836 bytes --]

=== modified file 'src/fns.c'
--- src/fns.c	2014-05-21 03:49:58 +0000
+++ src/fns.c	2014-06-24 10:04:45 +0000
@@ -50,7 +50,9 @@
 static Lisp_Object Qmd5, Qsha1, Qsha224, Qsha256, Qsha384, Qsha512;
 
 static bool internal_equal (Lisp_Object, Lisp_Object, int, bool, Lisp_Object);
-\f
+static void validate_subarray (Lisp_Object, Lisp_Object, Lisp_Object,
+			       ptrdiff_t, EMACS_INT *, EMACS_INT *);
+
 DEFUN ("identity", Fidentity, Sidentity, 1, 1, 0,
        doc: /* Return the argument unchanged.  */)
   (Lisp_Object arg)
@@ -232,6 +234,7 @@
 \(exclusive).  If START1 is nil, it defaults to 0, the beginning of
 the string; if END1 is nil, it defaults to the length of the string.
 Likewise, in string STR2, compare the part between START2 and END2.
+Like in `substring', negative values are counted from the end.
 
 The strings are compared by the numeric values of their characters.
 For instance, STR1 is "less than" STR2 if its first differing
@@ -244,43 +247,25 @@
   - 1 - N is the number of characters that match at the beginning.
 If string STR1 is greater, the value is a positive number N;
   N - 1 is the number of characters that match at the beginning.  */)
-  (Lisp_Object str1, Lisp_Object start1, Lisp_Object end1, Lisp_Object str2, Lisp_Object start2, Lisp_Object end2, Lisp_Object ignore_case)
+  (Lisp_Object str1, Lisp_Object start1, Lisp_Object end1, Lisp_Object str2,
+   Lisp_Object start2, Lisp_Object end2, Lisp_Object ignore_case)
 {
-  register ptrdiff_t end1_char, end2_char;
-  register ptrdiff_t i1, i1_byte, i2, i2_byte;
+  EMACS_INT from1, to1, from2, to2;
+  ptrdiff_t i1, i1_byte, i2, i2_byte;
 
   CHECK_STRING (str1);
   CHECK_STRING (str2);
-  if (NILP (start1))
-    start1 = make_number (0);
-  if (NILP (start2))
-    start2 = make_number (0);
-  CHECK_NATNUM (start1);
-  CHECK_NATNUM (start2);
-  if (! NILP (end1))
-    CHECK_NATNUM (end1);
-  if (! NILP (end2))
-    CHECK_NATNUM (end2);
-
-  end1_char = SCHARS (str1);
-  if (! NILP (end1) && end1_char > XINT (end1))
-    end1_char = XINT (end1);
-  if (end1_char < XINT (start1))
-    args_out_of_range (str1, start1);
-
-  end2_char = SCHARS (str2);
-  if (! NILP (end2) && end2_char > XINT (end2))
-    end2_char = XINT (end2);
-  if (end2_char < XINT (start2))
-    args_out_of_range (str2, start2);
-
-  i1 = XINT (start1);
-  i2 = XINT (start2);
+
+  validate_subarray (str1, start1, end1, SCHARS (str1), &from1, &to1);
+  validate_subarray (str2, start2, end2, SCHARS (str2), &from2, &to2);
+
+  i1 = from1;
+  i2 = from2;
 
   i1_byte = string_char_to_byte (str1, i1);
   i2_byte = string_char_to_byte (str2, i2);
 
-  while (i1 < end1_char && i2 < end2_char)
+  while (i1 < to1 && i2 < to2)
     {
       /* When we find a mismatch, we must compare the
 	 characters, not just the bytes.  */
@@ -307,12 +292,8 @@
 
       if (! NILP (ignore_case))
 	{
-	  Lisp_Object tem;
-
-	  tem = Fupcase (make_number (c1));
-	  c1 = XINT (tem);
-	  tem = Fupcase (make_number (c2));
-	  c2 = XINT (tem);
+	  c1 = XINT (Fupcase (make_number (c1)));
+	  c2 = XINT (Fupcase (make_number (c2)));
 	}
 
       if (c1 == c2)
@@ -322,15 +303,15 @@
 	 past the character that we are comparing;
 	 hence we don't add or subtract 1 here.  */
       if (c1 < c2)
-	return make_number (- i1 + XINT (start1));
+	return make_number (- i1 + from1);
       else
-	return make_number (i1 - XINT (start1));
+	return make_number (i1 - from1);
     }
 
-  if (i1 < end1_char)
-    return make_number (i1 - XINT (start1) + 1);
-  if (i2 < end2_char)
-    return make_number (- i1 + XINT (start1) - 1);
+  if (i1 < to1)
+    return make_number (i1 - from1 + 1);
+  if (i2 < to2)
+    return make_number (- i1 + from1 - 1);
 
   return Qt;
 }


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: Too permissive compare-strings?
  2014-06-24 14:03 Too permissive compare-strings? Dmitry Antipov
@ 2014-06-25  3:33 ` Stefan Monnier
  0 siblings, 0 replies; 2+ messages in thread
From: Stefan Monnier @ 2014-06-25  3:33 UTC (permalink / raw)
  To: Dmitry Antipov; +Cc: Emacs development discussions

> Surprisingly, I also found that current implementation of this
> function silently allows invalid ranges. E.g. this is not an error:

> (compare-strings "test" 0 100 "testbed" 0 4)

Indeed, this is used in string-prefix-p (a common use case for
compare-strings):

  (eq t (compare-strings str1 nil nil
                         str2 0 (length str1) ignore-case)))

With your semantics, we'd have to compare the lengths of str1 and str2
before calling compare-strings.

I think it's too late to "fix".


        Stefan



^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2014-06-25  3:33 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-24 14:03 Too permissive compare-strings? Dmitry Antipov
2014-06-25  3:33 ` Stefan Monnier

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.