Re: mprotect(2) misbehaves when len == 0?

2010-02-12 Thread Jason McIntyre 
On Fri, Feb 12, 2010 at 10:57:32AM +0300, Vadim Zhukov wrote:
 On 12 February 2010 ?. 10:42:09 Jason McIntyre wrote:
  On Thu, Feb 11, 2010 at 08:24:52PM -0500, Ted Unangst wrote:
   The man page is likely missing the word no, as that makes a lot
   more sense than the current wording meaning positive action.
 
  well, can you fix that then, please?
  jmc
 
 Maybe it's better to say that this is not treated as error too? See the
 patch at the end of this letter.
 

i don;t know this stuff. but why would you expect an error in this
case? if we are saying that if len is 0, then this happens, why do
we need to say it doesn;t generate an error?

so, how important is it?

 
 Index: mprotect.2
 ===
 RCS file: /cvs/src/lib/libc/sys/mprotect.2,v
 retrieving revision 1.14
 diff -u -p -r1.14 mprotect.2
 --- mprotect.231 May 2007 19:19:33 -  1.14
 +++ mprotect.212 Feb 2010 07:55:38 -
 @@ -53,10 +53,11 @@ through
  .Fa len
  \- 1
  (inclusive).
 -If
 -.Fa len
 -is 0, then action will be taken on the page that contains
 -.Fa addr .
 +It is not an error to specify 0 as
 +.Fa len ,
 +but no action will be taken on the page that contains
 +.Fa addr
 +then .
  .Pp
  Not all implementations will guarantee protection on a page basis;
  the granularity of protection changes may be as large as an entire region.

this wording is not great. but i won;t suggest anything else until you
or someone answers the point above.

jmc

Re: mprotect(2) misbehaves when len == 0?

2010-02-12 Thread patrick keshishian 
On Fri, Feb 12, 2010 at 10:48:00AM +0300, Vadim Zhukov wrote:
 On 12 February 2010 P3. 03:31:22 Jordi Beltran Creix wrote:
  Wouldn't that return EINVAL unless the address is aligned to the page
  boundary?
 
 From the man page:
 The OpenBSD implementation of mprotect() does not require addr to be
 page-aligned. And code is agree, too.
 
 But the problem is not with the address itself, it's all about how to
 treat 0 passed as len parameter. I didn't checked behavior of other
 *BSD, but their mprotect(2) pages do not say anything about treating 0
 in len parameter at all. Linux manual is most clear: The function
 mprotect specifies the desired protection for the memory page(s)
 containing part or all of the interval [addr,addr+len-1].

I'm confused about the [addr,addr+len-1], cause based on
above wording (from Linux manual per you), len = 0 produces
a strange range.

As you specified in your original post, SUS says:

| The function mprotect() changes the access protections
| to be that specified by prot for those whole pages
| containing any part of the address space of the
| process starting at address addr and continuing for
| len bytes.

So if len = 0, then no pages are protected based on that
wording. Which, is what the OpenBSD man page says assuming
there was a typo and the word no is missing in the sentence:

| If len is 0, then action will be taken on the page
| that contains addr.

(should read)

| If len is 0, then no action will be taken on the page
| that contains addr.

--patrick

Re: mprotect(2) misbehaves when len == 0?

2010-02-12 Thread Jason McIntyre 
On Fri, Feb 12, 2010 at 11:27:35AM +0300, Vadim Zhukov wrote:
  
   Maybe it's better to say that this is not treated as error too? See
   the patch at the end of this letter.
 
  i don;t know this stuff. but why would you expect an error in this
  case? if we are saying that if len is 0, then this happens, why do
  we need to say it doesn;t generate an error?
 
 For example, another memory management function manual page, malloc(3), 
 explicitly specifies that it's not an error to specify zero in size 
 parameter.
 

can you quote me the bit of text? i cannot immediately see it.

 Such declaration make saves time from test compilation that proves it's 
 not an error. I think this is job for manual - avoiding such testing.
 

my point is that if we say that if len is 0 then no action is taken, it
is implicit that it is not an error to do so. so again i'm asking, is it
important to do so? could someone reading that page really think well,
no action is taken, but will it cause an error?

  so, how important is it?
 
   Index: mprotect.2
   ===
   RCS file: /cvs/src/lib/libc/sys/mprotect.2,v
   retrieving revision 1.14
   diff -u -p -r1.14 mprotect.2
   --- mprotect.231 May 2007 19:19:33 -  1.14
   +++ mprotect.212 Feb 2010 07:55:38 -
   @@ -53,10 +53,11 @@ through
.Fa len
\- 1
(inclusive).
   -If
   -.Fa len
   -is 0, then action will be taken on the page that contains
   -.Fa addr .
   +It is not an error to specify 0 as
   +.Fa len ,
   +but no action will be taken on the page that contains
   +.Fa addr
   +then .
.Pp
Not all implementations will guarantee protection on a page basis;
the granularity of protection changes may be as large as an entire
   region.
 
  this wording is not great. but i won;t suggest anything else until you
  or someone answers the point above.
 
 Sorry, I'm not even close to be called native English speaker. :(
 

assuming we must say it, i suggest:

It is not an error if
.Fa len
is 0,
though no action will be taken on the page that contains
.Fa addr .

jmc

Re: mprotect(2) misbehaves when len == 0?

2010-02-12 Thread Igor Sobrado 
On Fri, Feb 12, 2010 at 4:21 PM, Jason McIntyre j...@kerhand.co.uk wrote:
 On Fri, Feb 12, 2010 at 02:01:26PM +, Owain Ainsworth wrote:
 Index: sys/mprotect.2
 ===
 RCS file: /cvs/src/lib/libc/sys/mprotect.2,v
 retrieving revision 1.14
 diff -u -p -r1.14 mprotect.2
 --- sys/mprotect.231 May 2007 19:19:33 -  1.14
 +++ sys/mprotect.212 Feb 2010 14:00:46 -
 @@ -55,7 +55,7 @@ through
  (inclusive).
  If
  .Fa len
 -is 0, then action will be taken on the page that contains
 +is 0, then no action will be taken on the page that contains

 personally i would be tempted to simplify this to:

If
.Fa len
is 0, no action is taken...

Agreed, I think that this style is much simpler and easy to read.  I
prefer the change this way too.

Re: mprotect(2) misbehaves when len == 0?

2010-02-12 Thread Ted Unangst 
On Fri, Feb 12, 2010 at 10:21 AM, Jason McIntyre j...@kerhand.co.uk wrote:
 On Fri, Feb 12, 2010 at 02:01:26PM +, Owain Ainsworth wrote:

 saying that no action is taken does imply that it is not an error.
 -is 0, then action will be taken on the page that contains
 +is 0, then no action will be taken on the page that contains

 personally i would be tempted to simplify this to:

If
.Fa len
is 0, no action is taken...

Agreed.  If no action is taken, you don't need to specify where the
action doesn't happen. :)

Re: mprotect(2) misbehaves when len == 0?

2010-02-12 Thread Owain Ainsworth 
On Fri, Feb 12, 2010 at 03:21:33PM +, Jason McIntyre wrote:
 On Fri, Feb 12, 2010 at 02:01:26PM +, Owain Ainsworth wrote:
  
  saying that no action is taken does imply that it is not an error.
  
  -0-
  
  diff follows:
  
  Index: sys/mprotect.2
  ===
  RCS file: /cvs/src/lib/libc/sys/mprotect.2,v
  retrieving revision 1.14
  diff -u -p -r1.14 mprotect.2
  --- sys/mprotect.2  31 May 2007 19:19:33 -  1.14
  +++ sys/mprotect.2  12 Feb 2010 14:00:46 -
  @@ -55,7 +55,7 @@ through
   (inclusive).
   If
   .Fa len
  -is 0, then action will be taken on the page that contains
  +is 0, then no action will be taken on the page that contains
 
 personally i would be tempted to simplify this to:
 
   If
   .Fa len
   is 0, no action is taken...
 
 jmc

Works for me.

-0-
-- 
Those who in quarrels interpose, must often wipe a bloody nose.

mprotect(2) misbehaves when len == 0?

2010-02-11 Thread Vadim Zhukov 
Hello all.

mprotect(2) says: If len is 0, then action will be taken on the page
that contains addr. The reality says it's not so:

$ cat ~/cvs/mprotect_test.c

#include sys/types.h
#include sys/param.h
#include sys/mman.h

#include stdio.h
#include unistd.h

void
mymemset(char *p, int c, size_t len) {
size_t  i;
charcc;

cc = (char)(c  0xFF);
for (i = 0; i  len; i++) {
fputc('.', stderr);
p[i] = cc;
}
}

int
main(int argc, char **argv) {
long pgsz;
char*p;

pgsz = sysconf(_SC_PAGESIZE);
if ((p = (char*)mmap(NULL, pgsz * 3, PROT_NONE, MAP_ANON, -1, 0)) == 
NULL)
err(1, mmap);
if (mprotect(p + pgsz, 0, PROT_READ|PROT_WRITE) == -1)
err(1, mprotect);
fputs(writing to page 1: , stderr);
mymemset(p + pgsz, 0xCC, pgsz);
fputs(OK\nwriting to page 0: , stderr);
mymemset(p, 0xCC, pgsz);
fputs(OK (WTF?!)\nwriting to page 2: , stderr);
mymemset(p + (pgsz * 2), 0xCC, pgsz);
fputs(OK (WTF?!)\n, stderr);

return 1;
}
$ cc -O0 -ggdb -o mprotect_test mprotect_test.c
$ ./mprotect_test
writing to page 1: .Segmentation fault (core dumped)
$

I checked the SUS, and it is not clear: The mprotect() function shall
change the access protections to be that specified by prot for those
whole pages containing any part of the address space of the process
starting at address addr and continuing for len bytes.

So either sys_mprotect(), uvm_map_protect() or man page is not correct.
As I'm totally newbie in UVM, please, do not bite me for the patch
proposed below. At least, very much. :)

-- 
  Best wishes,
Vadim Zhukov

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?


@@ -778,6 +778,8 @@ sys_mprotect(struct proc *p, void *v, re

if ((prot  VM_PROT_ALL) != prot)
return (EINVAL);
+   if (size == 0)
+   size = PAGE_SIZE;
 
/*
 * align the address to a page boundary, and adjust the size accordingly

Re: mprotect(2) misbehaves when len == 0?

2010-02-11 Thread Jordi Beltran Creix 
Wouldn't that return EINVAL unless the address is aligned to the page boundary?

Re: mprotect(2) misbehaves when len == 0?

2010-02-11 Thread Ted Unangst 
The man page is likely missing the word no, as that makes a lot more
sense than the current wording meaning positive action.

2010/2/11 Vadim Zhukov persg...@gmail.com:
 Hello all.

 mprotect(2) says: If len is 0, then action will be taken on the page
 that contains addr. The reality says it's not so:

 $ cat ~/cvs/mprotect_test.c

 #include sys/types.h
 #include sys/param.h
 #include sys/mman.h

 #include stdio.h
 #include unistd.h

 void
 mymemset(char *p, int c, size_t len) {
size_t  i;
charcc;

cc = (char)(c  0xFF);
for (i = 0; i  len; i++) {
fputc('.', stderr);
p[i] = cc;
}
 }

 int
 main(int argc, char **argv) {
long pgsz;
char*p;

pgsz = sysconf(_SC_PAGESIZE);
if ((p = (char*)mmap(NULL, pgsz * 3, PROT_NONE, MAP_ANON, -1, 0)) ==
NULL)
err(1, mmap);
if (mprotect(p + pgsz, 0, PROT_READ|PROT_WRITE) == -1)
err(1, mprotect);
fputs(writing to page 1: , stderr);
mymemset(p + pgsz, 0xCC, pgsz);
fputs(OK\nwriting to page 0: , stderr);
mymemset(p, 0xCC, pgsz);
fputs(OK (WTF?!)\nwriting to page 2: , stderr);
mymemset(p + (pgsz * 2), 0xCC, pgsz);
fputs(OK (WTF?!)\n, stderr);

return 1;
 }
 $ cc -O0 -ggdb -o mprotect_test mprotect_test.c
 $ ./mprotect_test
 writing to page 1: .Segmentation fault (core dumped)
 $

 I checked the SUS, and it is not clear: The mprotect() function shall
 change the access protections to be that specified by prot for those
 whole pages containing any part of the address space of the process
 starting at address addr and continuing for len bytes.

 So either sys_mprotect(), uvm_map_protect() or man page is not correct.
 As I'm totally newbie in UVM, please, do not bite me for the patch
 proposed below. At least, very much. :)

 --
  Best wishes,
Vadim Zhukov

 A: Because it messes up the order in which people normally read text.
 Q: Why is top-posting such a bad thing?
 A: Top-posting.
 Q: What is the most annoying thing in e-mail?


 @@ -778,6 +778,8 @@ sys_mprotect(struct proc *p, void *v, re

if ((prot  VM_PROT_ALL) != prot)
return (EINVAL);
 +   if (size == 0)
 +   size = PAGE_SIZE;

/*
 * align the address to a page boundary, and adjust the size
accordingly