vc.el and CSSC

vc.el and CSSC

[Ulrich Mueller]

Hi,

while applying and testing the recent security fix for lib-src/vcdiff
(CVE-2008-1694), I've noticed that vc does not work with GNU CSSC.

The vcdiff script doesn't find the "get" command which CSSC installs
in /usr/libexec/cssc/:

/usr/libexec/emacs/22.2/i686-pc-linux-gnu/vcdiff: line 88: get: command not found

Also, commands like vc-sccs-checkout don't work because they don't
find "get" in PATH.

The patch included below (for CVS trunk) should fix it. However, I did
only some basic testing (checkout, checkin, diff).

Ulrich


--- lib-src/vcdiff	19 Apr 2008 10:11:34 +0200	1.17
+++ lib-src/vcdiff	19 Apr 2008 17:53:59 +0200	
@@ -27,7 +27,8 @@
 DIFF="diff"
 usage="$0: Usage: vcdiff [--brief] [-q] [-r<sid1>] [-r<sid2>] [diffopts] sccsfile..."
 
-PATH=$PATH:/usr/ccs/bin:/usr/sccs:/usr/xpg4/bin # common SCCS hangouts
+# common SCCS hangouts
+PATH=$PATH:/usr/libexec/cssc:/usr/ccs/bin:/usr/sccs:/usr/xpg4/bin
 
 echo="echo"
 sid1= sid2=
--- lisp/vc-hooks.el	19 Apr 2008 10:11:35 +0200	1.238
+++ lisp/vc-hooks.el	19 Apr 2008 17:55:59 +0200	
@@ -85,9 +85,11 @@
   :group 'vc)
 
 (defcustom vc-path
-  (if (file-directory-p "/usr/sccs")
-      '("/usr/sccs")
-    nil)
+  (append
+   (if (file-directory-p "/usr/libexec/cssc")
+       '("/usr/libexec/cssc"))
+   (if (file-directory-p "/usr/sccs")
+       '("/usr/sccs")))
   "List of extra directories to search for version control commands."
   :type '(repeat directory)
   :group 'vc)

Re: vc.el and CSSC

[Stefan Monnier]

> while applying and testing the recent security fix for lib-src/vcdiff
> (CVE-2008-1694), I've noticed that vc does not work with GNU CSSC.

> The vcdiff script doesn't find the "get" command which CSSC installs
> in /usr/libexec/cssc/:

> /usr/libexec/emacs/22.2/i686-pc-linux-gnu/vcdiff: line 88: get: command not found

> Also, commands like vc-sccs-checkout don't work because they don't
> find "get" in PATH.

> The patch included below (for CVS trunk) should fix it. However, I did
> only some basic testing (checkout, checkin, diff).

Thanks for the report.  The patch is very helpful.

Since this is a problem specific to SCCS (or CSSC), I think we should
try and fix it in vc-sccs.el rather than vc-hooks.el.  I.e. we should
also remove the (if (file-directory-p "/usr/sccs") '("/usr/sccs")) from
vc-hooks.

Can you check if the patch below fixes your problem as well?

I'd also be happy to get rid of vcdiff.  E.g. maybe use "sccs diffs" or
(re)write `vcdiff' in Elisp.


        Stefan


Index: lib-src/vcdiff
===================================================================
RCS file: /sources/emacs/emacs/lib-src/vcdiff,v
retrieving revision 1.17
diff -u -r1.17 vcdiff
--- lib-src/vcdiff	18 Apr 2008 18:24:29 -0000	1.17
+++ lib-src/vcdiff	19 Apr 2008 20:44:11 -0000
@@ -85,14 +85,14 @@
 	s.* | */s.*)
 		if
 			rev1=`mktemp /tmp/geta.XXXXXXXX`
-			get -s -p -k $sid1 "$f" > $rev1 &&
+			sccs get -s -p -k $sid1 "$f" > $rev1 &&
 			case $sid2 in
 			'')
 				workfile=`expr " /$f" : '.*/s.\(.*\)'`
 				;;
 			*)
 				rev2=`mktemp /tmp/getb.XXXXXXXX`
-				get -s -p -k $sid2 "$f" > $rev2
+				sccs get -s -p -k $sid2 "$f" > $rev2
 				workfile=$rev2
 			esac
 		then
Index: lisp/vc-hooks.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/vc-hooks.el,v
retrieving revision 1.238
diff -u -r1.238 vc-hooks.el
--- lisp/vc-hooks.el	18 Apr 2008 10:01:07 -0000	1.238
+++ lisp/vc-hooks.el	19 Apr 2008 20:44:11 -0000
@@ -84,10 +84,7 @@
   :type '(repeat string)
   :group 'vc)
 
-(defcustom vc-path
-  (if (file-directory-p "/usr/sccs")
-      '("/usr/sccs")
-    nil)
+(defcustom vc-path nil
   "List of extra directories to search for version control commands."
   :type '(repeat directory)
   :group 'vc)
Index: lisp/vc-sccs.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/vc-sccs.el,v
retrieving revision 1.45
diff -u -r1.45 vc-sccs.el
--- lisp/vc-sccs.el	11 Apr 2008 15:17:59 -0000	1.45
+++ lisp/vc-sccs.el	19 Apr 2008 20:44:11 -0000
@@ -90,17 +101,16 @@
 \f
 ;;; Properties of the backend
 
-(defun vc-sccs-revision-granularity ()
-     'file)
+(defun vc-sccs-revision-granularity () 'file)
 
 ;;;
 ;;; State-querying functions
 ;;;
 
-;;; The autoload cookie below places vc-sccs-registered directly into
-;;; loaddefs.el, so that vc-sccs.el does not need to be loaded for
-;;; every file that is visited.  The definition is repeated below
-;;; so that Help and etags can find it.
+;; The autoload cookie below places vc-sccs-registered directly into
+;; loaddefs.el, so that vc-sccs.el does not need to be loaded for
+;; every file that is visited.  The definition is repeated below
+;; so that Help and etags can find it.
 
 ;;;###autoload (defun vc-sccs-registered(f) (vc-default-registered 'SCCS f))
 (defun vc-sccs-registered (f) (vc-default-registered 'SCCS f))
@@ -181,6 +191,11 @@
 ;;; State-changing functions
 ;;;
 
+(defun vc-sccs-do-command (buffer okstatus command file-or-list &rest flags)
+  ;; (let ((load-path (append vc-sccs-path load-path)))
+  ;;   (apply 'vc-do-command buffer okstatus command file-or-list flags))
+  (apply 'vc-do-command buffer okstatus "sccs" file-or-list command flags))
+
 (defun vc-sccs-create-repo ()
   "Create a new SCCS repository."
   ;; SCCS is totally file-oriented, so all we have to do is make the directory
@@ -203,7 +218,7 @@
       (let ((vc-name
 	     (or project-file
 		 (format (car vc-sccs-master-templates) dirname basename))))
-	(apply 'vc-do-command nil 0 "admin" vc-name
+	(apply 'vc-sccs-do-command nil 0 "admin" vc-name
 	       (and rev (not (string= rev "")) (concat "-r" rev))
 	       "-fb"
 	       (concat "-i" (file-relative-name file))
@@ -211,7 +226,7 @@
 	       (vc-switches 'SCCS 'register)))
       (delete-file file)
       (if vc-keep-workfiles
-	  (vc-do-command nil 0 "get" (vc-name file))))))
+	  (vc-sccs-do-command nil 0 "get" (vc-name file))))))
 
 (defun vc-sccs-responsible-p (file)
   "Return non-nil if SCCS thinks it would be responsible for registering FILE."
@@ -223,15 +238,15 @@
 (defun vc-sccs-checkin (files rev comment)
   "SCCS-specific version of `vc-backend-checkin'."
   (dolist (file files)
-    (apply 'vc-do-command nil 0 "delta" (vc-name file)
+    (apply 'vc-sccs-do-command nil 0 "delta" (vc-name file)
 	   (if rev (concat "-r" rev))
 	   (concat "-y" comment)
 	   (vc-switches 'SCCS 'checkin))
     (if vc-keep-workfiles
-	(vc-do-command nil 0 "get" (vc-name file)))))
+	(vc-sccs-do-command nil 0 "get" (vc-name file)))))
 
 (defun vc-sccs-find-revision (file rev buffer)
-  (apply 'vc-do-command
+  (apply 'vc-sccs-do-command
 	 buffer 0 "get" (vc-name file)
 	 "-s" ;; suppress diagnostic output
 	 "-p"
@@ -263,7 +278,7 @@
 	  (and rev (or (string= rev "")
                        (not (stringp rev)))
                (setq rev nil))
-	  (apply 'vc-do-command nil 0 "get" (vc-name file)
+	  (apply 'vc-sccs-do-command nil 0 "get" (vc-name file)
 		 (if editable "-e")
 		 (and rev (concat "-r" (vc-sccs-lookup-triple file rev)))
 		 switches))))
@@ -279,13 +294,14 @@
 					   discard file)))
 		(error "Aborted"))
 	    (message "Removing revision %s from %s..." discard file)
-	    (vc-do-command nil 0 "rmdel" (vc-name file) (concat "-r" discard))
-	    (vc-do-command nil 0 "get" (vc-name file) nil))))
+	    (vc-sccs-do-command nil 0 "rmdel"
+                                (vc-name file) (concat "-r" discard))
+	    (vc-sccs-do-command nil 0 "get" (vc-name file) nil))))
 
 (defun vc-sccs-revert (file &optional contents-done)
   "Revert FILE to the version it was based on."
-  (vc-do-command nil 0 "unget" (vc-name file))
-  (vc-do-command nil 0 "get" (vc-name file))
+  (vc-sccs-do-command nil 0 "unget" (vc-name file))
+  (vc-sccs-do-command nil 0 "get" (vc-name file))
   ;; Checking out explicit revisions is not supported under SCCS, yet.
   ;; We always "revert" to the latest revision; therefore
   ;; vc-working-revision is cleared here so that it gets recomputed.
@@ -293,14 +309,16 @@
 
 (defun vc-sccs-steal-lock (file &optional rev)
   "Steal the lock on the current workfile for FILE and revision REV."
-  (vc-do-command nil 0 "unget" (vc-name file) "-n" (if rev (concat "-r" rev)))
-  (vc-do-command nil 0 "get" (vc-name file) "-g" (if rev (concat "-r" rev))))
+  (vc-sccs-do-command nil 0 "unget"
+                      (vc-name file) "-n" (if rev (concat "-r" rev)))
+  (vc-sccs-do-command nil 0 "get"
+                      (vc-name file) "-g" (if rev (concat "-r" rev))))
 
 (defun vc-sccs-modify-change-comment (files rev comment)
   "Modify (actually, append to) the change comments for FILES on a specified REV."
   (dolist (file files)
-    (vc-do-command nil 0 "cdc" (vc-name file) 
-		   (concat "-y" comment) (concat "-r" rev))))
+    (vc-sccs-do-command nil 0 "cdc" (vc-name file) 
+                        (concat "-y" comment) (concat "-r" rev))))
 
 \f
 ;;;
@@ -309,7 +327,7 @@
 
 (defun vc-sccs-print-log (files &optional buffer)
   "Get change log associated with FILES."
-  (vc-do-command buffer 0 "prs" (mapcar 'vc-name files)))
+  (vc-sccs-do-command buffer 0 "prs" (mapcar 'vc-name files)))
 
 (defun vc-sccs-wash-log ()
   "Remove all non-comment information from log output."

Re: vc.el and CSSC

[Ulrich Mueller]

>>>>> On Sat, 19 Apr 2008, Stefan Monnier wrote:

> Since this is a problem specific to SCCS (or CSSC), I think we
> should try and fix it in vc-sccs.el rather than vc-hooks.el.
> I.e. we should also remove the (if (file-directory-p "/usr/sccs")
> '("/usr/sccs")) from vc-hooks.

> Can you check if the patch below fixes your problem as well?

At least a quick test of checkout, checkin, and diff succeeds.

> -			get -s -p -k $sid1 "$f" > $rev1 &&
> +			sccs get -s -p -k $sid1 "$f" > $rev1 &&

> -				get -s -p -k $sid2 "$f" > $rev2
> +				sccs get -s -p -k $sid2 "$f" > $rev2

If you prefix the SCCS commands in vcdiff with "sccs", then you may
get rid of the path manipulation completely (tested here, too):

--- lib-src/vcdiff	19 Apr 2008 18:46:59 +0200	1.17
+++ lib-src/vcdiff	19 Apr 2008 23:04:07 +0200	
@@ -27,8 +27,6 @@
 DIFF="diff"
 usage="$0: Usage: vcdiff [--brief] [-q] [-r<sid1>] [-r<sid2>] [diffopts] sccsfile..."
 
-PATH=$PATH:/usr/ccs/bin:/usr/sccs:/usr/xpg4/bin # common SCCS hangouts
-
 echo="echo"
 sid1= sid2=
 

Ulrich

Re: vc.el and CSSC

[Ulrich Mueller]

>> Can you check if the patch below fixes your problem as well?

> At least a quick test of checkout, checkin, and diff succeeds.

On second thought, you have to be more careful here:

"get" and "sccs get" are not completely equivalent, but the "sccs"
variant prepends an "SCCS/s." to its filename arguments before passing
them to "get".

While CSSC seems to play nicely with it, I don't know if all SCCS
variants will be happy if the "sccs" wrapper is called with SCCS
master filenames. So probably vc-sccs-master-templates shouldn't be
used for filename manipulation in this case ...

But to play it safe, I would suggest to stay with the present form of
the commands, without the "sccs" wrapper.

Ulrich

Re: vc.el and CSSC

[Stefan Monnier]

>> Since this is a problem specific to SCCS (or CSSC), I think we
>> should try and fix it in vc-sccs.el rather than vc-hooks.el.
>> I.e. we should also remove the (if (file-directory-p "/usr/sccs")
>> '("/usr/sccs")) from vc-hooks.

>> Can you check if the patch below fixes your problem as well?

> At least a quick test of checkout, checkin, and diff succeeds.

Thanks, installed.


        Stefan

Re: vc.el and CSSC

[Stefan Monnier]

>>> Can you check if the patch below fixes your problem as well?
>> At least a quick test of checkout, checkin, and diff succeeds.
> On second thought, you have to be more careful here:

> "get" and "sccs get" are not completely equivalent, but the "sccs"
> variant prepends an "SCCS/s." to its filename arguments before passing
> them to "get".

> While CSSC seems to play nicely with it, I don't know if all SCCS
> variants will be happy if the "sccs" wrapper is called with SCCS
> master filenames.

I'm tempted to leave it in and wait for people to complain, at which
point we'll know better what the problem really looks like.

> So probably vc-sccs-master-templates shouldn't be used for filename
> manipulation in this case ...

Yes, maybe we good get rid of vc-sccs-master-templates, which would be
good as well.

> But to play it safe, I would suggest to stay with the present form of
> the commands, without the "sccs" wrapper.

There's plenty of time to fix potential breakage on the trunk,


        Stefan