[PATCH] Ensure that sendfile doesn't try to send beyond the end of the source file. Added some tests, and ensured that the same result is observed under both Linux and OSv.

2016-12-04 Thread Rick Payne 
Fixes #815

Signed-off-by: Rick Payne 
---
 fs/vfs/main.cc| 15 +++
 tests/tst-sendfile.cc | 28 +++-
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/fs/vfs/main.cc b/fs/vfs/main.cc
index 8fecc08..924bfad 100644
--- a/fs/vfs/main.cc
+++ b/fs/vfs/main.cc
@@ -2039,6 +2039,21 @@ int sendfile(int out_fd, int in_fd, off_t *_offset, 
size_t count)
 offset = lseek(in_fd, 0, SEEK_CUR);
 }
 
+// Constrain count to the extent of the file...
+struct stat st;
+if (fstat(in_fd, ) < 0) {
+return -1;
+} else {
+if (offset >= st.st_size) {
+return 0;
+} else if ((offset + count) >= st.st_size) {
+count = st.st_size - offset;
+if (count == 0) {
+return 0;
+}
+}
+}
+
 size_t bytes_to_mmap = count + (offset % mmu::page_size);
 off_t offset_for_mmap =  align_down(offset, (off_t)mmu::page_size);
 
diff --git a/tests/tst-sendfile.cc b/tests/tst-sendfile.cc
index 2dcb302..0eb800a 100644
--- a/tests/tst-sendfile.cc
+++ b/tests/tst-sendfile.cc
@@ -163,6 +163,22 @@ int test_sendfile_on_socket(off_t *offset, size_t count)
 return listener_result == 0 ? ret : -1;
 }
 
+int test_extents(int testfile_readfd, size_t offset, size_t count)
+{
+const char *out_file = "testdata_sendfile_output";
+off_t off;
+int write_fd = open(out_file, O_RDWR | O_TRUNC | O_CREAT, S_IRWXU);
+if (write_fd == -1) {
+printf("\topen() failed with error message = %s\n",strerror(errno));
+return -1;
+}
+lseek(testfile_readfd, 0 , SEEK_CUR);
+off = offset;
+int ret  = sendfile(write_fd, testfile_readfd, , count);
+close(write_fd);
+return ret;
+}
+
 int main()
 {
 int ret;
@@ -200,12 +216,22 @@ int main()
 report(test_functions[i](offset_p[j], count_array[k]) == 
count_array[k], message.c_str());
 }
 offset = 0;
-   report(lseek(testfile_readfd, 0, SEEK_SET) == 0, "set readfd to 
beginning of file");
+report(lseek(testfile_readfd, 0, SEEK_SET) == 0, "set readfd to 
beginning of file");
 report(test_functions[i](offset_p[j], size_test_file) == 
size_test_file, "copy entire file");
 printf("\n\n");
 }
 }
 
+// Test extents...
+report(lseek(testfile_readfd, 0, SEEK_SET) == 0, "set readfd to beginning 
of file");
+report(test_extents(testfile_readfd, 0, size_test_file + 1) == 
size_test_file, "file size extent");
+report(lseek(testfile_readfd, 0, SEEK_SET) == 0, "set readfd to beginning 
of file");
+report(test_extents(testfile_readfd, 10, size_test_file) == 
(size_test_file - 10), "file size extent with offset");
+report(lseek(testfile_readfd, 0, SEEK_SET) == 0, "set readfd to beginning 
of file");
+report(test_extents(testfile_readfd, size_test_file - 1, 1) == 1, "file 
size extent with offset (tail)");
+report(lseek(testfile_readfd, 0, SEEK_SET) == 0, "set readfd to beginning 
of file");
+report(test_extents(testfile_readfd, size_test_file, 1) == 0, "file size 
extent with offset (tail, no bytes)");
+
 /* force sendfile to fail in rest of test cases */
 ret = sendfile(100, testfile_readfd, NULL, 10);
 report(ret == -1 && errno == EBADF, "test for bad out_fd");
-- 
2.7.4

-- 
You received this message because you are subscribed to the Google Groups "OSv 
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Problems in sendfile?

2016-12-04 Thread Nadav Har'El 
On Sun, Dec 4, 2016 at 9:05 PM, Rick Payne  wrote:

>
> What about something like the following. Not sure if you favour a
> different approach rather than checking the extents in the sendfile code,
> but this certainly gets me moving forward again.
>

Your patch looks good - my only question was whether we should be using
fstat() here or, because we already have the file structure in_fp, we could
also use in_fp->stat() (I think) directly. But maybe you're right and it's
better to just use the general system calls like you did - we are already
using lseek() and mmap() in the same code, and not some internal functions,
so why not use fstat() as well?

>
> I can work on some tst-sendfile code too if you want - but don’t want to
> duplicate effort if you’re already looking into it… Will send proper patch
> after discussion.
>

That would be great. I only open the bug tracker issue (please write "Fixes
#..." on the commit message so it will be closed), but wasn't working on
any code or test.

Thanks!
Nadav.


>
> Cheers,
> Rick
>
> diff --git a/fs/vfs/main.cc b/fs/vfs/main.cc
> index 8fecc08..b611ff2 100644
> --- a/fs/vfs/main.cc
> +++ b/fs/vfs/main.cc
> @@ -2039,6 +2039,21 @@ int sendfile(int out_fd, int in_fd, off_t *_offset,
> size_t count)
>  offset = lseek(in_fd, 0, SEEK_CUR);
>  }
>
> +// Constrain count to the extent of the file...
> +struct stat st;
> +if (fstat(in_fd, ) < 0) {
> +  return -1;
> +} else {
> +  if (offset >= st.st_size) {
> +return 0;
> +  } else if ((offset + count) >= st.st_size) {
> +count = st.st_size - offset;
> +if (count == 0) {
> +  return 0;
> +}
> +  }
> +}
> +
>  size_t bytes_to_mmap = count + (offset % mmu::page_size);
>  off_t offset_for_mmap =  align_down(offset, (off_t)mmu::page_size);
>
>
>

-- 
You received this message because you are subscribed to the Google Groups "OSv 
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Problems in sendfile?

2016-12-04 Thread Roman Shaposhnik 
On Sun, Dec 4, 2016 at 12:34 AM, Avi Kivity  wrote:
> On 12/04/2016 04:00 AM, Roman Shaposhnik wrote:
>>
>> sendfile is a pretty Linux-centric API and I'm sure it isn't implemented
>> by OSv.
>
>
> OSv aims to be Linux compatible, and does implement sendfile() (though not
> without problems, as we see).
>
> sendfile() originated in Solaris, IIRC, and variants exist in the BSDs as
> well.

Good info -- I stand corrected!

Thanks,
Roman.

-- 
You received this message because you are subscribed to the Google Groups "OSv 
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Problems in sendfile?

2016-12-04 Thread Rick Payne 

What about something like the following. Not sure if you favour a different 
approach rather than checking the extents in the sendfile code, but this 
certainly gets me moving forward again.

I can work on some tst-sendfile code too if you want - but don’t want to 
duplicate effort if you’re already looking into it… Will send proper patch 
after discussion.

Cheers,
Rick

diff --git a/fs/vfs/main.cc b/fs/vfs/main.cc
index 8fecc08..b611ff2 100644
--- a/fs/vfs/main.cc
+++ b/fs/vfs/main.cc
@@ -2039,6 +2039,21 @@ int sendfile(int out_fd, int in_fd, off_t *_offset, 
size_t count)
 offset = lseek(in_fd, 0, SEEK_CUR);
 }

+// Constrain count to the extent of the file...
+struct stat st;
+if (fstat(in_fd, ) < 0) {
+  return -1;
+} else {
+  if (offset >= st.st_size) {
+return 0;
+  } else if ((offset + count) >= st.st_size) {
+count = st.st_size - offset;
+if (count == 0) {
+  return 0;
+}
+  }
+}
+
 size_t bytes_to_mmap = count + (offset % mmu::page_size);
 off_t offset_for_mmap =  align_down(offset, (off_t)mmu::page_size);


-- 
You received this message because you are subscribed to the Google Groups "OSv 
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[COMMIT osv master] pthread_mutex_trylock: Return EBUSY not -EBUSY

2016-12-04 Thread Commit Bot 

From: Rick Payne 
Committer: Nadav Har'El 
Branch: master

pthread_mutex_trylock: Return EBUSY not -EBUSY

Signed-off-by: Rick Payne 
Message-Id: <1480693088-22453-1-git-send-email-ri...@rossfell.co.uk>

---
diff --git a/libc/pthread.cc b/libc/pthread.cc
--- a/libc/pthread.cc
+++ b/libc/pthread.cc
@@ -430,7 +430,7 @@ int pthread_mutex_lock(pthread_mutex_t *m)
 int pthread_mutex_trylock(pthread_mutex_t *m)
 {
 if (!from_libc(m)->try_lock()) {
-return -EBUSY;
+return EBUSY;
 }
 return 0;
 }

--
You received this message because you are subscribed to the Google Groups "OSv 
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Problems in sendfile?

2016-12-04 Thread Nadav Har'El 
On Sun, Dec 4, 2016 at 11:00 AM, Nadav Har'El  wrote:

>
>
> I am guessing that the problem might be that the file is smaller than 1
> gigabyte, and the sendfile() is reading more than mmap() mapped and thus
> getting a sigsegv. We need to see what Linux does in that case - does
> sendfile() succeed and just write less? Or does it fail with EOVERFLOW? I
> don't remember.
>

It looks like the erlang code is calling sendfile with a large value
>> (larger than the size of the actual file being sent). I see the sendfile
>> code calculating the bytes to map, then mapping that area. It then crashes
>> during the write:
>>
>
I just noticed that you did already verify that the given count is larger
than the actual file size. So clearly, the code expected that to work, but
it doesn't on OSv.
So I opened an issue https://github.com/cloudius-systems/osv/issues/815 for
fixing the case of sendfile() beyond the actual file size.

Nadav.

-- 
You received this message because you are subscribed to the Google Groups "OSv 
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Problems in sendfile?

2016-12-04 Thread Avi Kivity 

On 12/04/2016 04:00 AM, Roman Shaposhnik wrote:

sendfile is a pretty Linux-centric API and I'm sure it isn't implemented by OSv.


OSv aims to be Linux compatible, and does implement sendfile() (though 
not without problems, as we see).


sendfile() originated in Solaris, IIRC, and variants exist in the BSDs 
as well.



Given that this is erlang and it must support platforms that don't
have sendfile(2)
I suggest you look into how to build it for POSIX (as opposed to
Linux) environment

Thanks,
Roman.

On Sat, Dec 3, 2016 at 5:06 PM, Rick Payne  wrote:

The erlang app that I’m hacking is sending out some data to a socket and its 
using sendfile to do this. This is causing OSv to abort:

(gdb) bt
#0  processor::cli_hlt () at arch/x64/processor.hh:248
#1  0x00209cf6 in arch::halt_no_interrupts () at arch/x64/arch.hh:48
#2  0x0049983a in osv::halt () at arch/x64/power.cc:24
#3  0x0022d3c5 in abort (fmt=0xa1555d "Aborted\n") at runtime.cc:130
#4  0x0022d290 in abort () at runtime.cc:96
#5  0x006a49c1 in osv::generate_signal (siginfo=..., 
ef=0x80007f40e068) at libc/signal.cc:128
#6  0x006a4aa1 in osv::handle_mmap_fault (addr=35184720220160, sig=7, 
ef=0x80007f40e068) at libc/signal.cc:139
#7  0x003c6363 in mmu::vm_sigbus (addr=35184720220160, 
ef=0x80007f40e068) at core/mmu.cc:1323
#8  0x003c7715 in mmu::file_vma::fault (this=0xad436c80, 
addr=35184720220160, ef=0x80007f40e068) at core/mmu.cc:1691
#9  0x003c6513 in mmu::vm_fault (addr=35184720220160, 
ef=0x80007f40e068) at core/mmu.cc:1342
#10 0x004899bb in page_fault (ef=0x80007f40e068) at 
arch/x64/mmu.cc:38
#11 
#12 0x0047e905 in repmovsq (n=, src=, 
dest=) at arch/x64/string.cc:90
#13 memcpy_repmov_old_ssse3 (dest=0x88898000, src=0x200014c01000, 
n=4096) at arch/x64/string.cc:270
#14 0x00676d29 in uiomove (cp=0x88898000, n=4096, 
uio=0x205e9c70) at fs/vfs/subr_uio.cc:62
#15 0x0023d78e in m_uiotombuf (uio=0x205e9c70, how=2, len=33397, 
align=0, min_size=2048, flags=0)
 at bsd/sys/kern/uipc_mbuf.cc:1847
#16 0x00245753 in sosend_generic (so=0xa252e200, addr=0x0, 
uio=0x205e9c70, top=0x0, control=0x0, flags=0, td=0x0)
 at bsd/sys/kern/uipc_socket.cc:1047
#17 0x00245ece in sosend (so=0xa252e200, addr=0x0, 
uio=0x205e9c70, top=0x0, control=0x0, flags=0, td=0x0)
 at bsd/sys/kern/uipc_socket.cc:1282
#18 0x00253d41 in socket_file::write (this=0xad1a1f80, 
uio=0x205e9c70, flags=0) at bsd/sys/kern/sys_socket.cc:91
#19 0x0067d3a0 in sys_write (fp=0xad1a1f80, iov=0x205e9d10, 
niov=1, offset=-1, count=0x205e9d00)
 at fs/vfs/vfs_syscalls.cc:311
#20 0x0064c09a in pwrite (fd=31, buf=0x200014c0, count=1073741823, 
offset=-1) at fs/vfs/main.cc:387
#21 0x0064c12b in write (fd=31, buf=0x200014c0, count=1073741823) 
at fs/vfs/main.cc:405
#22 0x0064f003 in sendfile (out_fd=31, in_fd=32, 
_offset=0x223889e8, count=1073741823) at fs/vfs/main.cc:2051
#23 0x11616d64 in efile_sendfile (errInfo=0x2238894c, in_fd=32, 
out_fd=31, offset=0x223889e8, nbytes=0x205e9e78,
 hdtl=0x0) at drivers/unix/unix_efile.c:930
#24 0x1163527a in invoke_sendfile (data=0x22388918) at 
drivers/common/efile_drv.c:1917
#25 0x115a98eb in async_main (arg=0x2098f0c0) at 
beam/erl_async.c:509
#26 0x116c0615 in thr_wrapper (vtwd=0x201fe810) at 
pthread/ethread.c:114
#27 0x0069eebf in 
pthread_private::pthread::::operator()(void) const 
(__closure=0xa0007f39e700)
 at libc/pthread.cc:114
#28 0x006a1a22 in std::_Function_handler >::_M_invoke(const std::_Any_data &) 
(__functor=...) at /usr/include/c++/5/functional:1871
#29 0x0044cb1c in std::function::operator()() const 
(this=0x80007f409070) at /usr/include/c++/5/functional:2267
#30 0x005bfac4 in sched::thread::main (this=0x80007f409040) at 
core/sched.cc:1171
#31 0x005bbca6 in sched::thread_main_c (t=0x80007f409040) at 
arch/x64/arch-switch.hh:164
#32 0x00489793 in thread_main () at arch/x64/entry.S:113

It looks like the erlang code is calling sendfile with a large value (larger 
than the size of the actual file being sent). I see the sendfile code 
calculating the bytes to map, then mapping that area. It then crashes during 
the write:

930 retval = sendfile(out_fd, in_fd, offset, SENDFILE_CHUNK_SIZE);
#22 0x0064f003 in sendfile (out_fd=31, in_fd=32, 
_offset=0x223889e8, count=1073741823) at fs/vfs/main.cc:2051
2051auto ret = write(out_fd, src + (offset % PAGESIZE), count);
(gdb) p/x count
$20 = 0x3fff

Any ideas?

Cheers,
Rick

--
You received this