Hi Jeremy, I'm afraid this is indeed caused by the talloc tidyup as one can perfectly reproduce it wih the following patch for smbtorture (call bin/smbtorture ncacn_np:lo local.file.file_lines_parse). I guess we need to set up some warning signs around file_lines_parse() expressing that it takes talloced content and that one should not free the separated lines array separately. Cheers, Guenther On 09/11/2020 19:55, Jeremy Allison via samba wrote:> On Tue, Nov 10, 2020 at 07:41:56AM +1300, Andrew Bartlett wrote: >> On Mon, 2020-11-09 at 08:55 -0800, Jeremy Allison via samba wrote: >>> On Mon, Nov 09, 2020 at 04:01:47PM +0100, Benedikt Kale? via samba >>> wrote: >>>> Dear List, >>>> >>>> I updated to samba 4.12.10-SerNet-Debian-9.buster from 4.12.9 >>>> and did a >>>> >>>> gluster volume set volume performance.write-behind off >>>> >>>> Now I get this in /var/log/syslog and randomly people don't get >>>> access >>>> to a shae. >>> >>> Can you load the debug symbols please ? That should give us >>> more data on where this is failing. >>> >>> Thanks ! >>> >>> Jeremy. >> >> Jeremy, >> >> To lock this down properly we might need to put (#include) the vfs >> module into a cmocka test, mocking the various gluster calls so we >> cover these codepaths. > > I'm just worried my talloc tidyups might be at fault, > but I honestly can't see where. > > Once we get a backtrace (and if it's not my fault :-) > I'm going to be happy to throw this over to Guenther, > Sachin and Anoop for test coverage :-). >-- G?nther Deschner GPG-ID: 8EE11688 Red Hat gdeschner at redhat.com Samba Team gd at samba.org -------------- next part -------------- A non-text attachment was scrubbed... Name: test.patch Type: text/x-patch Size: 1628 bytes Desc: not available URL: <http://lists.samba.org/pipermail/samba/attachments/20201110/d055946c/test.bin>
On Tue, Nov 10, 2020 at 06:20:45PM +0100, G?nther Deschner wrote:> Hi Jeremy, > > I'm afraid this is indeed caused by the talloc tidyup as one can perfectly > reproduce it wih the following patch for smbtorture (call bin/smbtorture > ncacn_np:lo local.file.file_lines_parse). I guess we need to set up some > warning signs around file_lines_parse() expressing that it takes talloced > content and that one should not free the separated lines array separately.Dammit, I'm *so sorry* for that mistake. I looked quickly at file_lines_parse() and saw it do the: ret = talloc_zero_array(mem_ctx, char *, i+2); but then missed the: talloc_steal(ret, p); below. That's a *horrible* internal API to expose to callers.
On Tue, Nov 10, 2020 at 09:58:36AM -0800, Jeremy Allison via samba wrote:> On Tue, Nov 10, 2020 at 06:20:45PM +0100, G?nther Deschner wrote: > > Hi Jeremy, > > > > I'm afraid this is indeed caused by the talloc tidyup as one can perfectly > > reproduce it wih the following patch for smbtorture (call bin/smbtorture > > ncacn_np:lo local.file.file_lines_parse). I guess we need to set up some > > warning signs around file_lines_parse() expressing that it takes talloced > > content and that one should not free the separated lines array separately. > > Dammit, I'm *so sorry* for that mistake. I looked quickly > at file_lines_parse() and saw it do the: > > ret = talloc_zero_array(mem_ctx, char *, i+2); > > but then missed the: > > talloc_steal(ret, p); > > below. That's a *horrible* internal API to > expose to callers.Yes, this: "'p' will be freed on error, and otherwise will be made a child of the returned array" char **file_lines_parse(char *p, size_t size, int *numlines, TALLOC_CTX *mem_ctx) is what I missed :-(. God, that's a *horrible* API side-effect and completely unintuitive :-(. Thanks *SO* much for adding a test for that. Do you want to propose an MR with this test and the additional fix for vfs_glusterfs and I'll RB+ and push. Sorry once again.
On Tue, Nov 10, 2020 at 06:20:45PM +0100, G?nther Deschner wrote:> Hi Jeremy, > > I'm afraid this is indeed caused by the talloc tidyup as one can perfectly > reproduce it wih the following patch for smbtorture (call bin/smbtorture > ncacn_np:lo local.file.file_lines_parse). I guess we need to set up some > warning signs around file_lines_parse() expressing that it takes talloced > content and that one should not free the separated lines array separately. > > Cheers, > GuentherGuenther, here's my change that goes on top of your test. I'll submit a MR with both our patches, if you could test with a glusterfs setup I'd appreciate it. Thanks, Jeremy.
On Tue, Nov 10, 2020 at 10:29:26AM -0800, Jeremy Allison via samba wrote:> On Tue, Nov 10, 2020 at 06:20:45PM +0100, G?nther Deschner wrote: > > Hi Jeremy, > > > > I'm afraid this is indeed caused by the talloc tidyup as one can perfectly > > reproduce it wih the following patch for smbtorture (call bin/smbtorture > > ncacn_np:lo local.file.file_lines_parse). I guess we need to set up some > > warning signs around file_lines_parse() expressing that it takes talloced > > content and that one should not free the separated lines array separately. > > > > Cheers, > > Guenther > > Guenther, here's my change that goes on top of your test. > > I'll submit a MR with both our patches, if you could > test with a glusterfs setup I'd appreciate it.Never mind, I see your patch is designed to cause a crash :-).