[Tarantool-patches] [PATCH 01/16] 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
---
 src/lua/fio.lua       |   3 +-
 test/app/fio.result   | 160 ++++++++++++++++++++++++------------------
 test/app/fio.test.lua |  67 ++++++++++--------
 3 files changed, 129 insertions(+), 101 deletions(-)

diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index 954c44cdf..5db8bdea3 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -101,8 +101,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 20cfe345d..8b5716658 100644
--- a/test/app/fio.result
+++ b/test/app/fio.result
@@ -1380,76 +1380,6 @@ fio.unlink(tmpfile)
 ---
 - true
 ...
-tmp1 = fio.pathjoin(tmpdir, "tmp1")
----
-...
-tmp2= fio.pathjoin(tmpdir, "tmp2")
----
-...
-test_run:cmd("setopt delimiter ';'")
----
-- true
-...
-function write_file(name, odd)
-    local fh = fio.open(name, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, tonumber('0777', 8))
-    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 ''");
----
-- true
-...
-fh1 = write_file(tmp1)
----
-...
-fh2 = write_file(tmp2)
----
-...
-fiber = require('fiber')
----
-...
-digest = require('digest')
----
-...
-str = fh1:read()
----
-...
-fh1:seek(0)
----
-- 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
----
-- true
-...
-fio.unlink(tmp1)
----
-- true
-...
-fio.unlink(tmp2)
----
-- true
-...
 fio.rmdir(tmpdir)
 ---
 - true
@@ -1602,3 +1532,93 @@ tmpdir = nil
 os.setenv('TMPDIR', old_tmpdir)
 ---
 ...
+--
+-- 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)
+---
+- true
+- 0
+...
+fd2:write('2'), fd2:seek(0)
+---
+- true
+- 0
+...
+res1, res2 = nil
+---
+...
+do                                                                              \
+    fiber.new(function() res1 = fd1:read() end)                                 \
+    fiber.new(function() res2 = fd2:read() end)                                 \
+end
+---
+...
+_ = test_run:wait_cond(function() return res1 and res2 end)
+---
+...
+assert(res1 == '1')
+---
+- true
+...
+assert(res2 == '2')
+---
+- true
+...
+--
+-- The same with pread().
+--
+res1, res2 = nil
+---
+...
+do                                                                              \
+    fiber.new(function() res1 = fd1:pread(1, 0) end)                            \
+    fiber.new(function() res2 = fd2:pread(1, 0) end)                            \
+end
+---
+...
+_ = test_run:wait_cond(function() return res1 and res2 end)
+---
+...
+assert(res1 == '1')
+---
+- true
+...
+assert(res2 == '2')
+---
+- true
+...
+fd1:close()
+---
+- true
+...
+fd2:close()
+---
+- true
+...
+fio.rmtree(tmpdir)
+---
+- true
+...
diff --git a/test/app/fio.test.lua b/test/app/fio.test.lua
index d1a20e8e5..085d72110 100644
--- a/test/app/fio.test.lua
+++ b/test/app/fio.test.lua
@@ -442,35 +442,6 @@ 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' }, tonumber('0777', 8))
-    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)
 
 --
@@ -526,3 +497,41 @@ fio.tempdir()
 tmpdir = nil
 
 os.setenv('TMPDIR', old_tmpdir)
+
+--
+-- 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.new(function() res1 = fd1:read() end)                                 \
+    fiber.new(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.new(function() res1 = fd1:pread(1, 0) end)                            \
+    fiber.new(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)