* 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.