[Tarantool-patches] [PATCH 01/15] fio: don't use shared buffer in pread()

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

fio:pread() used buffer.IBUF_SHARED, which might be reused after a
yield. As a result, if pread() was called from 2 different fibers
or in parallel with something else using IBUF_SHARED, it would
turn the buffer into garbage for all parallel usages.

The same problem existed for read(), and was fixed in
c7c24f841322528c17714482d150b769d0afcddb ("fio: Fix race condition
in fio.read"). But apparently pread() was missed.

What is worse, the original commit's test passed even without the
fix from that commit. Because it didn't check the results of
read()s called from 2 fibers.

The patch fixes pread() and adds a test covering both read() and
pread(). The old test from the original commit is dropped.

Follow up #3187

(cherry picked from commit 24d86294b693867afd02a8847649026ce6b6fb4f)
---
 src/lua/fio.lua       |   3 +-
 test/app/fio.result   | 103 ++++++++++++++++++++++++++----------------
 test/app/fio.test.lua |  68 ++++++++++++++++------------
 3 files changed, 103 insertions(+), 71 deletions(-)

diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index 748efed14..a47bf2192 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -95,8 +95,7 @@ fio_methods.pread = function(self, buf, size, offset)
     if not ffi.istype(const_char_ptr_t, buf) then
         offset = size
         size = buf
-        tmpbuf = buffer.IBUF_SHARED
-        tmpbuf:reset()
+        tmpbuf = buffer.ibuf()
         buf = tmpbuf:reserve(size)
     end
     local res, err = internal.pread(self.fh, buf, size, offset)
diff --git a/test/app/fio.result b/test/app/fio.result
index f5820c199..03134b6af 100644
--- a/test/app/fio.result
+++ b/test/app/fio.result
@@ -7,6 +7,9 @@ ffi = require 'ffi'
 buffer = require 'buffer'
 ---
 ...
+fiber = require('fiber')
+---
+...
 test_run = require('test_run').new()
 ---
 ...
@@ -1179,90 +1182,110 @@ fio.unlink(tmpfile)
 ---
 - true
 ...
-tmp1 = fio.pathjoin(tmpdir, "tmp1")
+fio.rmdir(tmpdir)
 ---
+- true
 ...
-tmp2= fio.pathjoin(tmpdir, "tmp2")
+--
+-- gh-4439: mktree error handling fix - creation of existing file/directory
+--
+fio.mktree('/dev/null')
 ---
+- false
+- 'Error creating directory /dev/null: File exists'
 ...
-test_run:cmd("setopt delimiter ';'")
+fio.mktree('/dev/null/dir')
 ---
-- true
+- false
+- 'Error creating directory /dev/null: File exists'
 ...
-function write_file(name, odd)
-    local fh = fio.open(name, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, 0777)
-    if odd then
-        fh:write(string.rep('1', 100))
-    else
-        fh:write(string.rep('2', 100))
-    end
-    fh:write(name)
-    fh:seek(0)
-    return fh
-end;
+--
+-- read() and pread() should not use a shared buffer so as not to clash with
+-- other fibers during yield.
+--
+rights = tonumber('0777', 8)
 ---
 ...
-test_run:cmd("setopt delimiter ''");
+flags = {'O_RDWR', 'O_TRUNC', 'O_CREAT'}
 ---
-- true
 ...
-fh1 = write_file(tmp1)
+tmpdir = fio.tempdir()
 ---
 ...
-fh2 = write_file(tmp2)
+file1 = fio.pathjoin(tmpdir, 'file1')
 ---
 ...
-fiber = require('fiber')
+file2 = fio.pathjoin(tmpdir, 'file2')
 ---
 ...
-digest = require('digest')
+fd1 = fio.open(file1, flags, rights)
 ---
 ...
-str = fh1:read()
+fd2 = fio.open(file2, flags, rights)
 ---
 ...
-fh1:seek(0)
+fd1:write('1'), fd1:seek(0)
 ---
+- true
 - 0
 ...
-hash = digest.crc32(str)
+fd2:write('2'), fd2:seek(0)
 ---
+- true
+- 0
 ...
-ch = fiber.channel(1)
+res1, res2 = nil
 ---
 ...
-f1 = fiber.create(function() str = fh1:read() ch:put(digest.crc32(str)) end)
+do                                                                              \
+    fiber.create(function() res1 = fd1:read() end)                              \
+    fiber.create(function() res2 = fd2:read() end)                              \
+end
 ---
 ...
-f2 = fiber.create(function() str = fh2:read() end)
+_ = test_run:wait_cond(function() return res1 and res2 end)
 ---
 ...
-ch:get() == hash
+assert(res1 == '1')
 ---
 - true
 ...
-fio.unlink(tmp1)
+assert(res2 == '2')
 ---
 - true
 ...
-fio.unlink(tmp2)
+--
+-- The same with pread().
+--
+res1, res2 = nil
+---
+...
+do                                                                              \
+    fiber.create(function() res1 = fd1:pread(1, 0) end)                         \
+    fiber.create(function() res2 = fd2:pread(1, 0) end)                         \
+end
+---
+...
+_ = test_run:wait_cond(function() return res1 and res2 end)
+---
+...
+assert(res1 == '1')
 ---
 - true
 ...
-fio.rmdir(tmpdir)
+assert(res2 == '2')
 ---
 - true
 ...
---
--- gh-4439: mktree error handling fix - creation of existing file/directory
---
-fio.mktree('/dev/null')
+fd1:close()
 ---
-- false
-- 'Error creating directory /dev/null: File exists'
+- true
 ...
-fio.mktree('/dev/null/dir')
+fd2:close()
 ---
-- false
-- 'Error creating directory /dev/null: File exists'
+- true
+...
+fio.rmtree(tmpdir)
+---
+- true
 ...
diff --git a/test/app/fio.test.lua b/test/app/fio.test.lua
index b2c53e3e3..4e0a2e87f 100644
--- a/test/app/fio.test.lua
+++ b/test/app/fio.test.lua
@@ -1,6 +1,7 @@
 fio = require 'fio'
 ffi = require 'ffi'
 buffer = require 'buffer'
+fiber = require('fiber')
 test_run = require('test_run').new()
 -- umask
 
@@ -381,38 +382,47 @@ fio.path.lexists(link)
 
 fio.unlink(link)
 fio.unlink(tmpfile)
-tmp1 = fio.pathjoin(tmpdir, "tmp1")
-tmp2= fio.pathjoin(tmpdir, "tmp2")
-test_run:cmd("setopt delimiter ';'")
-function write_file(name, odd)
-    local fh = fio.open(name, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, 0777)
-    if odd then
-        fh:write(string.rep('1', 100))
-    else
-        fh:write(string.rep('2', 100))
-    end
-    fh:write(name)
-    fh:seek(0)
-    return fh
-end;
-test_run:cmd("setopt delimiter ''");
-fh1 = write_file(tmp1)
-fh2 = write_file(tmp2)
-fiber = require('fiber')
-digest = require('digest')
-str = fh1:read()
-fh1:seek(0)
-hash = digest.crc32(str)
-ch = fiber.channel(1)
-f1 = fiber.create(function() str = fh1:read() ch:put(digest.crc32(str)) end)
-f2 = fiber.create(function() str = fh2:read() end)
-ch:get() == hash
-
-fio.unlink(tmp1)
-fio.unlink(tmp2)
 fio.rmdir(tmpdir)
 --
 -- gh-4439: mktree error handling fix - creation of existing file/directory
 --
 fio.mktree('/dev/null')
 fio.mktree('/dev/null/dir')
+
+--
+-- read() and pread() should not use a shared buffer so as not to clash with
+-- other fibers during yield.
+--
+rights = tonumber('0777', 8)
+flags = {'O_RDWR', 'O_TRUNC', 'O_CREAT'}
+tmpdir = fio.tempdir()
+file1 = fio.pathjoin(tmpdir, 'file1')
+file2 = fio.pathjoin(tmpdir, 'file2')
+fd1 = fio.open(file1, flags, rights)
+fd2 = fio.open(file2, flags, rights)
+fd1:write('1'), fd1:seek(0)
+fd2:write('2'), fd2:seek(0)
+
+res1, res2 = nil
+do                                                                              \
+    fiber.create(function() res1 = fd1:read() end)                              \
+    fiber.create(function() res2 = fd2:read() end)                              \
+end
+_ = test_run:wait_cond(function() return res1 and res2 end)
+assert(res1 == '1')
+assert(res2 == '2')
+--
+-- The same with pread().
+--
+res1, res2 = nil
+do                                                                              \
+    fiber.create(function() res1 = fd1:pread(1, 0) end)                         \
+    fiber.create(function() res2 = fd2:pread(1, 0) end)                         \
+end
+_ = test_run:wait_cond(function() return res1 and res2 end)
+assert(res1 == '1')
+assert(res2 == '2')
+
+fd1:close()
+fd2:close()
+fio.rmtree(tmpdir)
-- 
2.24.3 (Apple Git-128)