From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 949F46EC56; Sat, 20 Mar 2021 03:43:23 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 949F46EC56 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1616201003; bh=JleAFuSsulGsgL+jz8vPj+4T7q3eHKa0B/S9MRddBto=; h=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=uqUg6ERNZFEkjbdmOrmuqNrP6zUxSq84Ko9b0JJgnUQb9TDEQZQ7QoddO/zbXv8YI g1clH32r3+0n9Wqbf82XSq6ezjrQQ2C9nDimN/20ifw6U4mQWnGNQYgoKaQeTE8ozw EdH8v13lZ56yau2RD7zahoY5mkv5kJx0U81RWMm8= Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C9DA76EC56 for ; Sat, 20 Mar 2021 03:42:49 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C9DA76EC56 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1lNPhY-00076U-SW; Sat, 20 Mar 2021 03:42:49 +0300 To: tarantool-patches@dev.tarantool.org, gorcunov@gmail.com, sergepetrenko@tarantool.org Date: Sat, 20 Mar 2021 01:42:31 +0100 Message-Id: <72141a12d0ea2cca70563695c9e32890f1b9dc0d.1616200860.git.v.shpilevoy@tarantool.org> X-Mailer: git-send-email 2.24.3 (Apple Git-128) In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD95D6E7CC48CB1F5F146FB1591D6F88157B67AD80B9C5634CD182A05F5380850401B3C445713336828F73FB49561E9E08C667DB57DA8DAE9345934EBB13A7E4585 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE79D9C320A40CA82D4EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063719513F03911326708638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C81A23F326053F827BB7116ABAE74875B1F337A0770A70E3DA471835C12D1D9774AD6D5ED66289B5259CC434672EE6371117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23DF004C9065253843057739F23D657EF2B13377AFFFEAFD26923F8577A6DFFEA7CB24F08513AFFAC7993EC92FD9297F6715571747095F342E857739F23D657EF2BD5E8D9A59859A8B6E5DA268271FB0930089D37D7C0E48F6C5571747095F342E857739F23D657EF2B6825BDBE14D8E70227F01F88B6EF2528BD9CCCA9EDD067B1EDA766A37F9254B7 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2BBE337FB72E923155C0AF1600DCBC20BD3311767B7455A33C61B31002A7AAE1F X-C1DE0DAB: C20DE7B7AB408E4181F030C43753B8186998911F362727C414F749A5E30D975C81A23F326053F827BB7116ABAE74875B1F337A0770A70E3D9C2B6934AE262D3EE7EAB7254005DCED7532B743992DF240BDC6A1CF3F042BAD6DF99611D93F60EFF532FBFD8162E58C699F904B3F4130E343918A1A30D5E7FCCB5012B2E24CD356 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34EA882B598A2098112A7D4B02B8F9755F84029EB191A94BA067FEA52C7B3606A6741B5AF647B1B9951D7E09C32AA3244C8041DFD0E72A7FB0C94FC7784D05E87DE646F07CC2D4F3D8FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj8xHC0Ak4ylbalk8furxA+A== X-Mailru-Sender: 689FA8AB762F73936BC43F508A0638224560A19215779A89DDA9C2639313A5EB3841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: [Tarantool-patches] [PATCH 01/16] fio: don't use shared buffer in pread() X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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)