Subject: Re: copyinstr() with a zero-length buffer
To: None <tech-kern@netbsd.org>
From: Charles M. Hannum <root@ihack.net>
List: tech-kern
Date: 11/02/1999 12:13:58
I observe the following additional problems:

* m68k returns 0 in this case, which is incorrect.  [Patch included.]

* mips treats maxlen==0 as maxlen==2^32.  [Patch included.]

* ns32k copystr() (but not copy{in,out}str()) returns 0.

* powerpc doesn't check whether the saved length pointer is non-null.
  It also uses EACCES rather then EFAULT.

* sh3 is schizo about checking the pointer, and will also look before
  the buffer (possibly causing an EFAULT).  [Patch included.]

* vax treats maxlen==0 as maxlen==2^32 (at least in the non-locc
  case).  It also doesn't save the length in EFAULT and ENAMETOOLONG
  cases.

(We should perhaps consider whether it even makes sense for the 4th
argument to be null.  If not, we could just remove the checks and make
the code slightly faster.)

Index: m68k/m68k/copy.s
===================================================================
RCS file: /cvsroot/syssrc/sys/arch/m68k/m68k/copy.s,v
retrieving revision 1.34
diff -c -2 -r1.34 copy.s
*** copy.s	1999/05/01 19:17:06	1.34
--- copy.s	1999/11/02 17:09:32
***************
*** 249,255 ****
  	movl	sp@(4),a0		| a0 = fromaddr
  	movl	sp@(8),a1		| a1 = toaddr
! 	clrl	d0
  	movl	sp@(12),d1		| count
  	beq	Lcsdone			| nothing to copy
  	subql	#1,d1			| predecrement for dbeq
  Lcsloop:
--- 249,256 ----
  	movl	sp@(4),a0		| a0 = fromaddr
  	movl	sp@(8),a1		| a1 = toaddr
! 	moveq	#ENAMETOOLONG,d0	| ran out of space
  	movl	sp@(12),d1		| count
  	beq	Lcsdone			| nothing to copy
+ 	clrl	d0
  	subql	#1,d1			| predecrement for dbeq
  Lcsloop:
***************
*** 282,286 ****
  	movl	sp@(4),a0		| a0 = fromaddr
  	movl	sp@(8),a1		| a1 = toaddr
! 	clrl	d0
  	movl	sp@(12),d1		| count
  	beq	Lcisdone		| nothing to copy
--- 283,287 ----
  	movl	sp@(4),a0		| a0 = fromaddr
  	movl	sp@(8),a1		| a1 = toaddr
! 	moveq	#ENAMETOOLONG,d0	| ran out of space
  	movl	sp@(12),d1		| count
  	beq	Lcisdone		| nothing to copy
***************
*** 321,325 ****
  	movl	sp@(4),a0		| a0 = fromaddr
  	movl	sp@(8),a1		| a1 = toaddr
! 	clrl	d0
  	movl	sp@(12),d1		| count
  	beq	Lcosdone		| nothing to copy
--- 322,326 ----
  	movl	sp@(4),a0		| a0 = fromaddr
  	movl	sp@(8),a1		| a1 = toaddr
! 	moveq	#ENAMETOOLONG,d0	| ran out of space
  	movl	sp@(12),d1		| count
  	beq	Lcosdone		| nothing to copy
Index: mips/mips/locore.S
===================================================================
RCS file: /cvsroot/syssrc/sys/arch/mips/mips/locore.S,v
retrieving revision 1.77
diff -c -2 -r1.77 locore.S
*** locore.S	1999/09/25 00:00:38	1.77
--- locore.S	1999/11/02 17:09:41
***************
*** 561,564 ****
--- 561,565 ----
  LEAF(copystr)
  	move	t0, a2
+ 	beq	a2, zero, 4f
  1:
  	lbu	v0, 0(a0)
***************
*** 569,572 ****
--- 570,574 ----
  	bne	a2, zero, 1b			# less than maxlen
  	addu	a1, a1, 1
+ 4:
  	li	v0, ENAMETOOLONG		# run out of space
  2:
***************
*** 592,595 ****
--- 594,598 ----
  	sw	v0, U_PCB_ONFAULT(v1)
  	move	t0, a2
+ 	beq	a2, zero, 4f
  1:
  	lbu	v0, 0(a0)
***************
*** 600,603 ****
--- 603,607 ----
  	bne	a2, zero, 1b
  	addu	a1, a1, 1
+ 4:
  	li	v0, ENAMETOOLONG
  2:
***************
*** 623,626 ****
--- 627,631 ----
  	sw	v0, U_PCB_ONFAULT(v1)
  	move	t0, a2
+ 	beq	a2, zero, 4f
  1:
  	lbu	v0, 0(a0)
***************
*** 631,634 ****
--- 636,640 ----
  	bne	a2, zero, 1b
  	addu	a1, a1, 1
+ 4:
  	li	v0, ENAMETOOLONG
  2:
Index: sh3/sh3/Locore.c
===================================================================
RCS file: /cvsroot/syssrc/sys/arch/sh3/sh3/Locore.c,v
retrieving revision 1.1
diff -c -2 -r1.1 Locore.c
*** Locore.c	1999/09/13 10:31:26	1.1
--- Locore.c	1999/11/02 17:09:42
***************
*** 252,277 ****
  
  	while (cnt--) {
! 		if ((*to++ = *from++) == 0)
! 			break;
  	}
  
! 	*lencopied = from - from_top;
! 
! 	if (cnt == 0) {
! 		if (to >= (char *)VM_MAXUSER_ADDRESS)
! 			rc = EFAULT;
! 		else
! 			rc = ENAMETOOLONG;
! 	} else
! 		rc = 0;
  
  	curpcb->pcb_onfault = 0;
  	return rc;
  
   Err999:
! 	curpcb->pcb_onfault = 0;
! 	if (lencopied != 0)
  		*lencopied = from - from_top;
! 
  	return EFAULT;
  }
--- 252,276 ----
  
  	while (cnt--) {
! 		if ((*to++ = *from++) == 0) {
! 			rc = 0;
! 			goto out;
! 		}
  	}
  
! 	if (to >= (char *)VM_MAXUSER_ADDRESS)
! 		rc = EFAULT;
! 	else
! 		rc = ENAMETOOLONG;
  
+ out:
+ 	if (lencopied)
+ 		*lencopied = from - from_top;
  	curpcb->pcb_onfault = 0;
  	return rc;
  
   Err999:
! 	if (lencopied)
  		*lencopied = from - from_top;
! 	curpcb->pcb_onfault = 0;
  	return EFAULT;
  }
***************
*** 305,332 ****
  
  	while (cnt--) {
! 		if ((*to++ = *from++) == 0)
! 			break;
  	}
- 
- 	if (lencopied != NULL)
- 		*lencopied = from - from_top;
  
! 	if (cnt == 0 && *(from - 1) != 0) {
! 		if (to >= (char *)VM_MAXUSER_ADDRESS)
! 			rc = EFAULT;
! 		else
! 			rc = ENAMETOOLONG;
! 	} else
! 		rc = 0;
  
  	curpcb->pcb_onfault = 0;
- 
  	return rc;
  
   Err999:
! 	curpcb->pcb_onfault = 0;
! 	if (lencopied != 0)
  		*lencopied = from - from_top;
! 
  	return EFAULT;
  }
--- 304,328 ----
  
  	while (cnt--) {
! 		if ((*to++ = *from++) == 0) {
! 			rc = 0;
! 			goto out;
! 		}
  	}
  
! 	if (to >= (char *)VM_MAXUSER_ADDRESS)
! 		rc = EFAULT;
! 	else
! 		rc = ENAMETOOLONG;
  
+ out:
+ 	if (lencopied)
+ 		*lencopied = from - from_top;
  	curpcb->pcb_onfault = 0;
  	return rc;
  
   Err999:
! 	if (lencopied)
  		*lencopied = from - from_top;
! 	curpcb->pcb_onfault = 0;
  	return EFAULT;
  }
***************
*** 350,365 ****
  
  	for (i = 0; i < maxlen; i++) {
! 		if ((*to++ = *from++) == NULL)
! 			break;
  	}
  
! 	if (i == maxlen) {
  		*lencopied = i;
! 		return ENAMETOOLONG;
! 	} else {
! 		if (lencopied)
! 			*lencopied = i + 1;
! 		return 0;
! 	}
  }
  
--- 346,359 ----
  
  	for (i = 0; i < maxlen; i++) {
! 		if ((*to++ = *from++) == NULL) {
! 			if (lencopied)
! 				*lencopied = i + 1;
! 			return (0);
! 		}
  	}
  
! 	if (lencopied)
  		*lencopied = i;
! 	return (ENAMETOOLONG);
  }