[9fans] segattach off-by-one?

[9fans] segattach off-by-one?

[Venkatesh Srinivas]

Hi,

This little program:

#include <u.h>
#include <libc.h>
#define SEGBASE ((char *) 0x10001001)
#define SEGSIZE 0x1000

void main(void) {
   segattach(0, "shared", SEGBASE, SEGSIZE);
   // Works fine (writing to 0x10001fff)
   *(char *) (SEGBASE + SEGSIZE - 2) = 'a';
   // Suicide! (writing to 0x10002000)
   *(char *) (SEGBASE + SEGSIZE - 1) = 'a';
}

However, segattach's manpage claims: "... and va+len is rounded up."
Shouldn't the second page here be mapped?

I propose this patch to /sys/src/9/port/segment.c::
--- segment.c.orig	2009-09-24 22:41:59.000000000 -0400
+++ segment.c	2009-09-24 22:38:25.000000000 -0400
@@ -641,6 +641,11 @@
 	int sno;
 	Segment *s, *os;
 	Physseg *ps;
+	ulong ova;
+
+	ova = va;
+	va = va&~(BY2PG-1);
+	len += (ova - va);

 	if(va != 0 && va >= USTKTOP)
 		error(Ebadarg);

This patch also prevents segattaching to the zero page, which I think
was worth doing...

Could people try this? Comments?

Thanks,
-- vs

Re: [9fans] segattach off-by-one?

[erik quanstrom]

the patch i posted yesterday was wrong.  i forgot about
automatic address assigment with segattach.  this works,
but perhaps it would be better to round va before checking
if va == 0?

/n/quanstro//sys/src/9/port/segment.c:642,648 - segment.c:642,648
  	Segment *s, *os;
  	Physseg *ps;

- 	if((va !=0 && va < BY2PG) || va >= USTKTOP)
+ 	if(va != 0 && va >= USTKTOP)
  		error(Ebadarg);

  	validaddr((ulong)name, 1, 0);

- erik

Re: [9fans] segattach off-by-one?

[erik quanstrom]

On Fri Sep 25 14:11:03 EDT 2009, me@acm.jhu.edu wrote:
> So in my example, va = 0x10001001, len = 0x1000. I understood that to
> mean [0x10001001, 0x10002001) was the newly-valid interval, which
> would mean 0x10002000 was a valid address...

i think you're misreading the man page.  from the man page
	Segattach creates a new memory segment, adds it to the call-
	ing process's address space, and returns its lowest address.
so you need to use the value returned, not the value
you gave it.  in this case 0x10001000 not 0x0001001.

also from the man page
	Va and len specify the position of the segment in the
	process's address space.  Va is rounded down to the nearest
	page boundary and va+len is rounded up.  The system does not
so va is rounded down (as you see) and the length
is rounded up, also as you see.

- erik

Re: [9fans] segattach off-by-one?

[Venkatesh Srinivas]

So in my example, va = 0x10001001, len = 0x1000. I understood that to
mean [0x10001001, 0x10002001) was the newly-valid interval, which
would mean 0x10002000 was a valid address...

The segattach manpage says va+len is 'rounded up'; wouldn't that mean
the expanded interval was [0x10001000, 0x10003000)? Or am I misreading
the manpage?

Thanks,
-- vs

Re: [9fans] segattach off-by-one?

[erik quanstrom]

(aside: "sig segattach" fails because nroff can elide
all leading space.  /n/sources/patch/signit

one could argue for replacing the \*? in
selecting files with \** but no functions
return a **.)

i think segattach is already correct.  consider this

#include <u.h>
#include <libc.h>

enum {
	Sz	= 0x1000,
};

void
main(void)
{
	uchar *base, *rb;

	base = (uchar*)0x10001001;
	rb = segattach(0, "shared", base, Sz - 10);
	print("%p\n", rb);
	base[0] = 'a';
	base[-1] = 'a';
	base[Sz - 2] = 'a';		/* note this does not fault, even though Sz - 10 was used */
>>	base[Sz - 1] = 'a';
	exits("");
}

% 8.out
10001000	# not 10001001.
81 8.out: checked 8 page table entries
8.out 81: suicide: sys: trap: fault write addr=0x10002000 pc=0x1073

if i were to have used rb and not base, this base[-1] would have
faulted but base[Sz - 1] would have been okay.  so i woud think
this is a misunderstanding of how segattach works.

if one wishes to disallow mapping the zero page this
condition should be changed:
	if(va != 0 && va >= USTKTOP)
		error(Ebadarg);
to
	if(va < BY2PG || va >= USTKTOP)

- erik