Re: segfault in SRFI-1 partition on non-list input

[ludo@gnu.org (Ludovic Courtès)]

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

Hi Julian,

"Julian Graham" <joolean@gmail.com> writes:

> It looks like scm_srfi1_partition in srfi/srfi-1.c fails to verify
> that it's being called on a real list, and so expressions like:
>
> (partition symbol? '(a b . c))
>
> cause Guile to segfault.  Attached is a patch against HEAD that adds validation.

> +  SCM_VALIDATE_LIST (2, list);

The probably is that `SCM_VALIDATE_LIST' uses `scm_ilength ()', which
attempts to traverse the list it's given.  Thus, it is undesirable to
use it here (and in many other places actually).

Instead, I propose the following patch, which doesn't add any list
traversal but doesn't catch circular lists.  What do you think?

Thanks,
Ludovic.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: The patch --]
[-- Type: text/x-patch, Size: 2051 bytes --]

diff --git a/srfi/srfi-1.c b/srfi/srfi-1.c
index 2989a25..0ce834a 100644
--- a/srfi/srfi-1.c
+++ b/srfi/srfi-1.c
@@ -1667,6 +1667,7 @@ SCM_DEFINE (scm_srfi1_partition, "partition", 2, 0, 0,
   /* In this implementation, the output lists don't share memory with
      list, because it's probably not worth the effort. */
   scm_t_trampoline_1 call = scm_trampoline_1(pred);
+  SCM orig_list = list;
   SCM kept = scm_cons(SCM_EOL, SCM_EOL);
   SCM kept_tail = kept;
   SCM dropped = scm_cons(SCM_EOL, SCM_EOL);
@@ -1675,8 +1676,15 @@ SCM_DEFINE (scm_srfi1_partition, "partition", 2, 0, 0,
   SCM_ASSERT(call, pred, 2, FUNC_NAME);
   
   for (; !SCM_NULL_OR_NIL_P (list); list = SCM_CDR(list)) {
-    SCM elt = SCM_CAR(list);
-    SCM new_tail = scm_cons(SCM_CAR(list), SCM_EOL);
+    SCM elt, new_tail;
+
+    /* LIST must be a proper list.
+       XXX: This does not ensure that LIST is not a circular list.  */
+    SCM_ASSERT (scm_is_pair (list), orig_list, 2, FUNC_NAME);
+
+    elt = SCM_CAR (list);
+    new_tail = scm_cons (SCM_CAR (list), SCM_EOL);
+
     if (scm_is_true (call (pred, elt))) {
       SCM_SETCDR(kept_tail, new_tail);
       kept_tail = new_tail;
diff --git a/test-suite/tests/srfi-1.test b/test-suite/tests/srfi-1.test
index 22c4a9a..8fe8097 100644
--- a/test-suite/tests/srfi-1.test
+++ b/test-suite/tests/srfi-1.test
@@ -1,6 +1,6 @@
 ;;;; srfi-1.test --- Test suite for Guile's SRFI-1 functions. -*- scheme -*-
 ;;;;
-;;;; Copyright 2003, 2004, 2005, 2006 Free Software Foundation, Inc.
+;;;; Copyright 2003, 2004, 2005, 2006, 2008 Free Software Foundation, Inc.
 ;;;;
 ;;;; This program is free software; you can redistribute it and/or modify
 ;;;; it under the terms of the GNU General Public License as published by
@@ -2068,7 +2068,11 @@
 				   (make-list 10000 1)))
       (lambda (even odd)
 	(and (= (length odd) 10000)
-	     (= (length even) 0))))))
+	     (= (length even) 0)))))
+
+  (pass-if-exception "with improper list"
+    exception:wrong-type-arg
+    (partition symbol? '(a b . c))))
 
 ;;
 ;; partition!