You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Oliver Deakin <ol...@googlemail.com> on 2009/09/29 16:40:33 UTC
Re: [classlib][luni] calling mmap
Tim Ellison wrote:
> Sorry for talking so long to raise this...
>
> On 16/Sep/2009 23:44, odeakin@apache.org wrote:
>
>> Author: odeakin
>> Date: Wed Sep 16 22:44:49 2009
>> New Revision: 815994
>>
>> URL: http://svn.apache.org/viewvc?rev=815994&view=rev
>> Log:
>> Apply patch for HARMONY-6315 ([classlib][nio] FileChannel.map throws IOException when called with size 0)
>>
>> Modified:
>> harmony/enhanced/classlib/trunk/modules/luni/src/main/java/org/apache/harmony/luni/platform/OSMemory.java
>> harmony/enhanced/classlib/trunk/modules/luni/src/main/native/luni/unix/OSMemoryLinux32.c
>> harmony/enhanced/classlib/trunk/modules/nio/src/test/java/common/org/apache/harmony/nio/tests/java/nio/MappedByteBufferTest.java
>>
>> Modified: harmony/enhanced/classlib/trunk/modules/luni/src/main/java/org/apache/harmony/luni/platform/OSMemory.java
>> URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/org/apache/harmony/luni/platform/OSMemory.java?rev=815994&r1=815993&r2=815994&view=diff
>> ==============================================================================
>> --- harmony/enhanced/classlib/trunk/modules/luni/src/main/java/org/apache/harmony/luni/platform/OSMemory.java (original)
>> +++ harmony/enhanced/classlib/trunk/modules/luni/src/main/java/org/apache/harmony/luni/platform/OSMemory.java Wed Sep 16 22:44:49 2009
>> @@ -551,6 +551,11 @@
>>
>> public long mmap(long fileDescriptor, long alignment, long size,
>> int mapMode) throws IOException {
>> + // if size is 0, call to mmap will fail, so return dummy address
>> + if (size == 0)
>> + {
>> + return 0;
>> + }
>> long address = mmapImpl(fileDescriptor, alignment, size, mapMode);
>> if (address == -1) {
>> throw new IOException();
>>
>
> Hmm, I'm not convinced this is the right solution.
>
> Making size == 0 a special case that always succeeds (even if it is
> returning a bogus address) bypasses all the other checks that the mmap
> function call would have made.
>
> For example, what if the file descriptor was invalid? or points to an
> unmappable file? Then I would expect an IOException.
>
I think (from the JIRA) that we were getting EINVAL back from the mmap
call with size 0, and that's why it was fixed before making the mmap
call. It would be good to know what the RI does in the case where size
is 0 and we're also mapping an invalid file - do we get an exception
thrown? Perhaps Catherine can clarify?
> There is nothing in the man pages to say that size = 0 is invalid.
>
No, but in the JIRA it indicates that we receive an EINVAL return code
when passing in size 0, so perhaps it is just not in the man page. The
man page I am looking at also didn't explicitly say negative values are
illegal, but I think we can assume that's true.
> Plus, there is now no path that will return -1 as the address, so you
> can remove the check (all the exceptions are raised in the native code).
>
That's true, I hadn't spotted that. I also noticed a bug in the native
code change where we allocate 100 chars for the error string but the
EAGAIN error message is 101 characters long (plus another 1 for the null
terminator) so we're off by 2 there. Perhaps a neater way to get these
error messages would be to use strerror() once we have the error number,
but for now Ill fix this array size bug and remove the redundant -1
check in the java code for now, and we can keep discussing the correct
behaviour here.
Regards,
Oliver
> Regards,
> Tim
>
>
>> Modified: harmony/enhanced/classlib/trunk/modules/luni/src/main/native/luni/unix/OSMemoryLinux32.c
>> URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/luni/src/main/native/luni/unix/OSMemoryLinux32.c?rev=815994&r1=815993&r2=815994&view=diff
>> ==============================================================================
>> --- harmony/enhanced/classlib/trunk/modules/luni/src/main/native/luni/unix/OSMemoryLinux32.c (original)
>> +++ harmony/enhanced/classlib/trunk/modules/luni/src/main/native/luni/unix/OSMemoryLinux32.c Wed Sep 16 22:44:49 2009
>> @@ -26,9 +26,11 @@
>> #include <sys/mman.h>
>> #include <errno.h>
>> #include <unistd.h>
>> +#include <string.h>
>> #include "vmi.h"
>> #include "OSMemory.h"
>> #include "IMemorySystem.h"
>> +#include "exceptions.h"
>>
>> #ifdef ZOS
>> #define FD_BIAS 1000
>> @@ -157,6 +159,7 @@
>> //PORT_ACCESS_FROM_ENV (env);
>> void *mapAddress = NULL;
>> int prot, flags;
>> + char errorString[100];
>>
>> // Convert from Java mapping mode to port library mapping mode.
>> switch (mmode)
>> @@ -174,12 +177,41 @@
>> flags = MAP_PRIVATE;
>> break;
>> default:
>> + sprintf(errorString, "Map mode %d not recognised", mmode);
>> + throwJavaIoIOException(env, errorString);
>> return -1;
>> }
>>
>> mapAddress = mmap(0, (size_t)(size&0x7fffffff), prot, flags, fd-FD_BIAS, (off_t)(alignment&0x7fffffff));
>> if (mapAddress == MAP_FAILED)
>> {
>> + switch (errno)
>> + {
>> + case EACCES:
>> + strcpy(errorString, "Call to mmap failed - a file descriptor refers to a non-regular file.");
>> + break;
>> + case EAGAIN:
>> + strcpy(errorString, "Call to mmap failed with error EAGAIN - the file has been locked, or too much memory has been locked.");
>> + break;
>> + case EBADF:
>> + strcpy(errorString, "Call to mmap failed with error EBADF - invalid file descriptor");
>> + break;
>> + case EINVAL:
>> + strcpy(errorString, "Call to mmap failed with error EINVAL - invalid start, length or offset.");
>> + break;
>> + case ENFILE:
>> + strcpy(errorString, "Call to mmap failed with error ENFILE - number of open files has reached the system limit.");
>> + break;
>> + case ENODEV:
>> + strcpy(errorString, "Call to mmap failed with error ENODEV - filesystem does not support memory mapping.");
>> + break;
>> + case ENOMEM:
>> + strcpy(errorString, "Call to mmap failed with error ENOMEM - no memory is available");
>> + break;
>> + default:
>> + sprintf(errorString, "Call to mmap returned with errno %d", errno);
>> + }
>> + throwJavaIoIOException(env, errorString);
>> return -1;
>> }
>> return (jlong) ((IDATA)mapAddress);
>>
>> Modified: harmony/enhanced/classlib/trunk/modules/nio/src/test/java/common/org/apache/harmony/nio/tests/java/nio/MappedByteBufferTest.java
>> URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/nio/src/test/java/common/org/apache/harmony/nio/tests/java/nio/MappedByteBufferTest.java?rev=815994&r1=815993&r2=815994&view=diff
>> ==============================================================================
>> --- harmony/enhanced/classlib/trunk/modules/nio/src/test/java/common/org/apache/harmony/nio/tests/java/nio/MappedByteBufferTest.java (original)
>> +++ harmony/enhanced/classlib/trunk/modules/nio/src/test/java/common/org/apache/harmony/nio/tests/java/nio/MappedByteBufferTest.java Wed Sep 16 22:44:49 2009
>> @@ -22,17 +22,19 @@
>> import java.io.FileOutputStream;
>> import java.io.IOException;
>> import java.io.RandomAccessFile;
>> +import java.nio.BufferUnderflowException;
>> import java.nio.ByteBuffer;
>> import java.nio.IntBuffer;
>> import java.nio.MappedByteBuffer;
>> import java.nio.channels.FileChannel;
>> +import java.nio.channels.NonWritableChannelException;
>> import java.nio.channels.FileChannel.MapMode;
>>
>> import junit.framework.TestCase;
>>
>> public class MappedByteBufferTest extends TestCase {
>>
>> - File tmpFile;
>> + File tmpFile, emptyFile;
>>
>> /**
>> * A regression test for failing to correctly set capacity of underlying
>> @@ -63,6 +65,62 @@
>> }
>>
>> /**
>> + * Regression for HARMONY-6315 - FileChannel.map throws IOException
>> + * when called with size 0
>> + *
>> + * @throws IOException
>> + */
>> + public void testEmptyBuffer() throws IOException {
>> + // Map empty file
>> + FileInputStream fis = new FileInputStream(emptyFile);
>> + FileChannel fc = fis.getChannel();
>> + MappedByteBuffer mmb = fc.map(FileChannel.MapMode.READ_ONLY, 0, fc.size());
>> +
>> + // check non-null
>> + assertNotNull("MappedByteBuffer created from empty file should not be null",
>> + mmb);
>> +
>> + // check capacity is 0
>> + int len = mmb.capacity();
>> + assertEquals("MappedByteBuffer created from empty file should have 0 capacity",
>> + 0, len);
>> +
>> + assertFalse("MappedByteBuffer from empty file shouldn't be backed by an array ",
>> + mmb.hasArray());
>> +
>> + try
>> + {
>> + byte b = mmb.get();
>> + fail("Calling MappedByteBuffer.get() on empty buffer should throw a BufferUnderflowException");
>> + }
>> + catch (BufferUnderflowException e)
>> + {
>> + // expected behaviour
>> + }
>> +
>> + // test expected exceptions thrown
>> + try
>> + {
>> + mmb = fc.map(FileChannel.MapMode.READ_WRITE, 0, fc.size());
>> + fail("Expected NonWritableChannelException to be thrown");
>> + }
>> + catch (NonWritableChannelException e)
>> + {
>> + // expected behaviour
>> + }
>> + try
>> + {
>> + mmb = fc.map(FileChannel.MapMode.PRIVATE, 0, fc.size());
>> + fail("Expected NonWritableChannelException to be thrown");
>> + }
>> + catch (NonWritableChannelException e)
>> + {
>> + // expected behaviour
>> + }
>> + fc.close();
>> + }
>> +
>> + /**
>> * @tests {@link java.nio.MappedByteBuffer#force()}
>> */
>> public void test_force() throws IOException {
>> @@ -146,5 +204,8 @@
>> fileChannel.write(byteBuffer);
>> fileChannel.close();
>> fileOutputStream.close();
>> +
>> + emptyFile = File.createTempFile("harmony", "test"); //$NON-NLS-1$//$NON-NLS-2$
>> + emptyFile.deleteOnExit();
>> }
>> }
>>
>>
>>
>>
>
>
--
Oliver Deakin
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Re: [classlib][luni] calling mmap
Posted by Catherine Hope <ca...@googlemail.com>.
I've uploaded a new patch that moves the check to PlatformAddressFactory and
retrieves the error messages with the portlib function as Mark suggested.
Thanks for all your help!
Cath
Re: [classlib][luni] calling mmap
Posted by Mark Hindess <ma...@googlemail.com>.
In message <25...@mail.gmail.com>,
sebb writes:
>
> On 29/09/2009, Oliver Deakin <ol...@googlemail.com> wrote:
> > Tim Ellison wrote:
> >
> > > On 29/Sep/2009 15:40, Oliver Deakin wrote:
> > >
[SNIP]
> > >
> > > > That's true, I hadn't spotted that. I also noticed a bug in the
> > > > native code change where we allocate 100 chars for the error
> > > > string but the EAGAIN error message is 101 characters long (plus
> > > > another 1 for the null terminator) so we're off by 2 there.
>
> Surely it's unsafe to use *any* string manipulation routines without
> checking that the target buffer has enough space?
Well, no. It is unsafe if you don't "control" all of the inputs. In
this case, we do since most of the inputs are just strcpy'd static
strings. The other two are contain '%d' format strings that I'm pretty
sure can't overflow the buffer.
I'm really puzzled why no one else has asked ... why are we copying
static strings? I must be missing something?
(They are "const char*" static strings and only seem to be used in a
"const char*" context so it makes no sense to copy them to a "char*".)
I also don't understand why the EACCESS string doesn't contain "with
error EACCES" so that it is consistent with the other cases. But this
is largely irrelevant since I don't think these strings add value
anyway... see below.
> Such buffer overflows are often exploitable to cause application
> crashes or worse.
Indeed. (This reminds me I must fix the one confusing one I found
in addDirsToPath in the launcher main.c. From memory, if the first
newPathToAdd is found then strLen is short by one separator because the
logic for calculating the length and doing the concatenation is not
consistent.)
> > > > Perhaps a neater way to get these error messages would be to use
> > > > strerror() once we have the error number,
I agree. Creating these contrived error messages is almost certainly
not productive since people are used to seeing the familiar
strerror/perror error messages. I don't see why we'd choose to create
custom error strings here and not do the same for all the other syscalls
in the classlib natives.
strerror isn't thread-safe so we probably shouldn't use this but rather
the XSI strerror_r. However, we seem to be making this more complicated
than it needs to be. Why not just something simple like:
PORT_ACCESS_FROM_ENV(env);
...
if(mapAddress == MAP_FAILED) {
hyerror_set_last_error(errno, HYPORT_ERROR_OPFAILED);
throwJavaIoIOException(env, hyerror_last_error_message());
return -1;
}
and let portlib do the work? We could make it more complicated and use
another buffer/sprintf/etc to add a prefix but I think the "Call to mmap
failed with error EBLAH" is rather redundant since the stacktrace from
the exception will clearly show the call to mmapImpl anyway.
I think the switch statement is just bloat and doesn't really add
anything apart from some unnecessary complexity and far more code to
read than is required to get the job done.
That's enough ranting for now. ;-)
Regards,
-Mark.
Re: [classlib][luni] calling mmap
Posted by sebb <se...@gmail.com>.
On 29/09/2009, Oliver Deakin <ol...@googlemail.com> wrote:
> Tim Ellison wrote:
>
> > On 29/Sep/2009 15:40, Oliver Deakin wrote:
> >
> >
> > > Tim Ellison wrote:
> > >
> > >
> > > > Making size == 0 a special case that always succeeds (even if it is
> > > > returning a bogus address) bypasses all the other checks that the mmap
> > > > function call would have made.
> > > >
> > > > For example, what if the file descriptor was invalid? or points to an
> > > > unmappable file? Then I would expect an IOException.
> > > >
> > > >
> > > I think (from the JIRA) that we were getting EINVAL back from the mmap
> > > call with size 0, and that's why it was fixed before making the mmap
> > > call. It would be good to know what the RI does in the case where size
> > > is 0 and we're also mapping an invalid file - do we get an exception
> > > thrown? Perhaps Catherine can clarify?
> > >
> > >
> >
> > Ok, I missed that. Then perhaps the special case should be on
> > PlatformAddressFactory#allocMap() so that we don't invoke
> the overhead
> > of managing the 'bogus' 0 as if it were a real address that needs
> > freeing etc.
> >
> >
> >
>
> Ok, that sounds reasonable - Ill take a look.
>
>
> >
> > >
> > > > There is nothing in the man pages to say that size = 0 is invalid.
> > > >
> > > >
> > > No, but in the JIRA it indicates that we receive an EINVAL return code
> > > when passing in size 0, so perhaps it is just not in the man page. The
> > > man page I am looking at also didn't explicitly say negative values are
> > > illegal, but I think we can assume that's true.
> > >
> > >
> >
> > The man pages I'm looking at declare size as a (size_t) which is usually
> > unsigned ;-)
> >
> >
>
> haha, ok that's true ;)
>
>
> >
> >
> > >
> > > > Plus, there is now no path that will return -1 as the address, so you
> > > > can remove the check (all the exceptions are raised in the native
> code).
> > > >
> > > >
> > > That's true, I hadn't spotted that. I also noticed a bug in the native
> > > code change where we allocate 100 chars for the error string but the
> > > EAGAIN error message is 101 characters long (plus another 1 for the null
> > > terminator) so we're off by 2 there.
Surely it's unsafe to use *any* string manipulation routines without
checking that the target buffer has enough space?
Such buffer overflows are often exploitable to cause application
crashes or worse.
> > > Perhaps a neater way to get these
> > > error messages would be to use strerror() once we have the error number,
> > > but for now Ill fix this array size bug and remove the redundant -1
> > > check in the java code for now, and we can keep discussing the correct
> > > behaviour here.
> > >
> > >
> >
> > I don't know what the correct behavior is in this case, I just mentioned
> > it because it looked odd.
> >
> >
>
> Understood - I think matching the RI here makes sense. I committed the
> removal of the -1 check and the char array size bug at repo revision
> r819983.
>
> Regards,
> Oliver
>
>
> > Regards,
> > Tim
> >
> >
> >
>
> --
> Oliver Deakin
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with number
> 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire
> PO6 3AU
>
>
Re: [classlib][luni] calling mmap
Posted by Oliver Deakin <ol...@googlemail.com>.
Tim Ellison wrote:
> On 29/Sep/2009 15:40, Oliver Deakin wrote:
>
>> Tim Ellison wrote:
>>
>>> Making size == 0 a special case that always succeeds (even if it is
>>> returning a bogus address) bypasses all the other checks that the mmap
>>> function call would have made.
>>>
>>> For example, what if the file descriptor was invalid? or points to an
>>> unmappable file? Then I would expect an IOException.
>>>
>> I think (from the JIRA) that we were getting EINVAL back from the mmap
>> call with size 0, and that's why it was fixed before making the mmap
>> call. It would be good to know what the RI does in the case where size
>> is 0 and we're also mapping an invalid file - do we get an exception
>> thrown? Perhaps Catherine can clarify?
>>
>
> Ok, I missed that. Then perhaps the special case should be on
> PlatformAddressFactory#allocMap() so that we don't invoke the overhead
> of managing the 'bogus' 0 as if it were a real address that needs
> freeing etc.
>
>
Ok, that sounds reasonable - Ill take a look.
>>> There is nothing in the man pages to say that size = 0 is invalid.
>>>
>> No, but in the JIRA it indicates that we receive an EINVAL return code
>> when passing in size 0, so perhaps it is just not in the man page. The
>> man page I am looking at also didn't explicitly say negative values are
>> illegal, but I think we can assume that's true.
>>
>
> The man pages I'm looking at declare size as a (size_t) which is usually
> unsigned ;-)
>
haha, ok that's true ;)
>
>>> Plus, there is now no path that will return -1 as the address, so you
>>> can remove the check (all the exceptions are raised in the native code).
>>>
>> That's true, I hadn't spotted that. I also noticed a bug in the native
>> code change where we allocate 100 chars for the error string but the
>> EAGAIN error message is 101 characters long (plus another 1 for the null
>> terminator) so we're off by 2 there. Perhaps a neater way to get these
>> error messages would be to use strerror() once we have the error number,
>> but for now Ill fix this array size bug and remove the redundant -1
>> check in the java code for now, and we can keep discussing the correct
>> behaviour here.
>>
>
> I don't know what the correct behavior is in this case, I just mentioned
> it because it looked odd.
>
Understood - I think matching the RI here makes sense. I committed the
removal of the -1 check and the char array size bug at repo revision
r819983.
Regards,
Oliver
> Regards,
> Tim
>
>
--
Oliver Deakin
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Re: [classlib][luni] calling mmap
Posted by Tim Ellison <t....@gmail.com>.
On 29/Sep/2009 15:40, Oliver Deakin wrote:
> Tim Ellison wrote:
>> Making size == 0 a special case that always succeeds (even if it is
>> returning a bogus address) bypasses all the other checks that the mmap
>> function call would have made.
>>
>> For example, what if the file descriptor was invalid? or points to an
>> unmappable file? Then I would expect an IOException.
>
> I think (from the JIRA) that we were getting EINVAL back from the mmap
> call with size 0, and that's why it was fixed before making the mmap
> call. It would be good to know what the RI does in the case where size
> is 0 and we're also mapping an invalid file - do we get an exception
> thrown? Perhaps Catherine can clarify?
Ok, I missed that. Then perhaps the special case should be on
PlatformAddressFactory#allocMap() so that we don't invoke the overhead
of managing the 'bogus' 0 as if it were a real address that needs
freeing etc.
>> There is nothing in the man pages to say that size = 0 is invalid.
>
> No, but in the JIRA it indicates that we receive an EINVAL return code
> when passing in size 0, so perhaps it is just not in the man page. The
> man page I am looking at also didn't explicitly say negative values are
> illegal, but I think we can assume that's true.
The man pages I'm looking at declare size as a (size_t) which is usually
unsigned ;-)
>> Plus, there is now no path that will return -1 as the address, so you
>> can remove the check (all the exceptions are raised in the native code).
>
> That's true, I hadn't spotted that. I also noticed a bug in the native
> code change where we allocate 100 chars for the error string but the
> EAGAIN error message is 101 characters long (plus another 1 for the null
> terminator) so we're off by 2 there. Perhaps a neater way to get these
> error messages would be to use strerror() once we have the error number,
> but for now Ill fix this array size bug and remove the redundant -1
> check in the java code for now, and we can keep discussing the correct
> behaviour here.
I don't know what the correct behavior is in this case, I just mentioned
it because it looked odd.
Regards,
Tim