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 8CEC96FC8F; Thu, 25 Mar 2021 00:25:12 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8CEC96FC8F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1616621112; bh=P4kCGloUkmYpQKvgnm9H5R455vjA3uis7xwIbSfvmg4=; 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=oc34UvrXJzsahV9Eo9PxBh1nWlbfZHfnNLG09KvYXUXzFKnTRxcys+Otwb/eynblW fn+1VIl1Yp940boQSjmmV4/OR84MgVFndCHN944hFEK3r/sNQHe6Y+T3Nt8Ao7JvOQ 0cLP0j7/TRbLeNoBuLFmc2iYHFlioNfFssz4u8Sk= Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 B17846FC8F for ; Thu, 25 Mar 2021 00:24:38 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B17846FC8F Received: by smtp48.i.mail.ru with esmtpa (envelope-from ) id 1lPAzV-0004ib-Il; Thu, 25 Mar 2021 00:24:38 +0300 To: tarantool-patches@dev.tarantool.org, kyukhin@tarantool.org Date: Wed, 24 Mar 2021 22:24:21 +0100 Message-Id: <81b10cd4d2d6b9657dc4af7ac92ff3e6fcd882ea.1616620860.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: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9064ADF4728AA0EE919EF87AC0718675712B828C563C11F50182A05F538085040F436AD0326EA85705F74DF5B4500078097BCAF327BB55B7568FA9A23E1C78C03 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7C3943609F29D73B3EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637B36BEFFD90A341448638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C5DD32608FC869F5DC955FE4EAA24D1CAAD55168E0510713DA471835C12D1D9774AD6D5ED66289B5278DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C317B107DEF921CE79117882F4460429728AD0CFFFB425014E868A13BD56FB6657D81D268191BDAD3DC09775C1D3CA48CFD94516D2695FEB51BA3038C0950A5D36C8A9BA7A39EFB766EC990983EF5C0329BA3038C0950A5D36D5E8D9A59859A8B6CCE505037BFD814176E601842F6C81A1F004C906525384307823802FF610243DF43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B3BBE47FD9DD3FB59A8DF7F3B2552694A2BEBFE083D3B9BA73A03B725D353964BB11811A4A51E3B096D1867E19FE14079BA9C0B312567BB23089D37D7C0E48F6CA18204E546F3947C83C798A30B85E16BC6EABA9B74D0DA47C8A9BA7A39EFB7666BA297DBC24807EA089D37D7C0E48F6C8AA50765F7900637B8F435DEDE9E76EBEFF80C71ABB335746BA297DBC24807EA27F269C8F02392CD6143B1329D1CCA5627F269C8F02392CD5571747095F342E88FB05168BE4CE3AF X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2368A440D3B0F6089093C9A16E5BC824AC8B6CDF511875BC4E8F7B195E1C97831148145A0506FFD487A82484DD75C3D90 X-C1DE0DAB: C20DE7B7AB408E4181F030C43753B8186998911F362727C414F749A5E30D975C5DD32608FC869F5DC955FE4EAA24D1CAAD55168E0510713D9C2B6934AE262D3EE7EAB7254005DCED7532B743992DF240BDC6A1CF3F042BAD6DF99611D93F60EF0417BEADF48D1460699F904B3F4130E343918A1A30D5E7FCCB5012B2E24CD356 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3433E9BC74ABA5769F42BA13D93EFDD18F6BAF100CAB630093FE61D94E164BD706DEB657C0FF3724061D7E09C32AA3244C92BA1087100A181A85DA573ED98CD7B1250262A5EE9971B0729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojjqzNotmU+gdq1rabIE6fww== X-Mailru-Sender: 504CC1E875BF3E7D9BC0E5172ADA3110A906815429F9D9605871705477D55043764C703329128C8607784C02288277CA03E0582D3806FB6A5317862B1921BA260ED6CFD6382C13A6112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: [Tarantool-patches] [PATCH 01/15] 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 (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)