Bug#974868: samba-vfs-modules: Still causing issues - at least on armv5tel/armel

[Michael Tokarev]

23.11.2022 00:23, ArtMG wrote:
..

Attachments:
* fruit-disable-useless-size_t-overflow-check.patch


Thanks for the patch Michael, I compiled and ran and although it got out of the 
disk size functions this time, it still slipped up with


I pushed this patch to debian samba package now.
The check there (which this patch removes) makes no sense on 64bits
(since the value is capped elsewhere) and is obviously wrong on
32bits, - for some reason it tries to calculate capacity in apples
when dealing with oranges all the way, and when sizes of apples and
oranges are entirely different on this platform, this test obviously
doe not do any good.  Also, this is a very specific issue affecting
only very specific use cases (with MacOS operability on 32bit), so
there should be no big harm done this way to make samba upstream
unhappy (if it were for something more widespread, I'd not push this
change to debian package).


../../source3/smbd/service.c:787(make_connection_snum)
make_connection_snum: canonicalize_connect_path failed for service 
TimeMachineBackup, path /mnt/HDD/TimeMachine


So this particular issue is gone now.  It might have other issues,
maybe due to size of size_t on 32bit in some other place, maybe due
to disk size limits, maybe something entirely different, but this
particular issue is gone.


I do appreciate you cutting this patch for this issue, however I feel like I've 
been down this road before with these issues. Push a fix here, and if a new 
issue doesn't pop up over there straight away, give it a version or three and 
it will eventually. Hence my reference to whack-a-mole.


It's not whack-a-mole really.  Whack-a-mole assumes there's just one
mole out there, which shows in different holes. But here, we have
many moles collected over the years of no testing.  You whack one
for good, but another dozen are chewing stuff somewhere else nearby,
ready to meet you.  This issue already whacked two which was in
the same place.


As Andrew points out, this is somewhat outlying as test-cases go. There are few 
people wanting to rebuild a 32-bit instance of their backup mechanism for each 
point revision of a large and actively-developed service like samba.


Well, once it's in debian, 32bit packages are provided automatically,
there's no need to rebuild them ;)


I see no reason, personally, to push for developer attention when the solution 
is as simple as 'switch to 64-bit OS', and then even OLD versions in all the 
repos work just fine. Although I do not have statistics to hand, I feel that we 
must be far enough along the migration curve on the journey from 32- to 64-bit 
systems, especially when it comes to a core-yet-commodity infrastructure 
service like samba, to warrant letting go of legacy.


It is not that simple, but can be viewed like this anyway.
Yes, 32bit world is dying, and the future is 64+bits, this is
not question whatsoever.  However, the question is *when* to
declare the death of 32bit world.  We either declare it dead
and stop building/providing samba on any 32bit platform entirely,
or we do not declare 32bit world dead, and provide a good service
there. What we do have now is neither one nor another, 32bit
world is not dead and samba is being provided for it, but the
quality of it on 32bits is, well, questionable.  So we either
should draw this line or fix bugs.

I dunno if it's good idea to push the change upstream though:
history tells me this is nearly hopeless to perform changes
in samba code even if the changes are small and obvious.. but
this is entirely different story.


Thanks for your good will and effort, nonetheless.


You're welcome.

Thanks!

/mjt

Bug#974868: samba-vfs-modules: Still causing issues - at least on armv5tel/armel

[ArtMG]

On 17 November 2022 8:38 PM, Michael Tokarev  wrote:
> Do you have ability to compile samba?  If yes (maybe with a debian package),
> can you please try the attached patch?  It removes the useless and actually
> wrong check for the off_t overflowing size_t.
> Attachments:
> * fruit-disable-useless-size_t-overflow-check.patch

Thanks for the patch Michael, I compiled and ran and although it got out of the 
disk size functions this time, it still slipped up with 

../../source3/smbd/service.c:787(make_connection_snum)
make_connection_snum: canonicalize_connect_path failed for service 
TimeMachineBackup, path /mnt/HDD/TimeMachine

I do appreciate you cutting this patch for this issue, however I feel like I've 
been down this road before with these issues. Push a fix here, and if a new 
issue doesn't pop up over there straight away, give it a version or three and 
it will eventually. Hence my reference to whack-a-mole. 

As Andrew points out, this is somewhat outlying as test-cases go. There are few 
people wanting to rebuild a 32-bit instance of their backup mechanism for each 
point revision of a large and actively-developed service like samba.

I see no reason, personally, to push for developer attention when the solution 
is as simple as 'switch to 64-bit OS', and then even OLD versions in all the 
repos work just fine. Although I do not have statistics to hand, I feel that we 
must be far enough along the migration curve on the journey from 32- to 64-bit 
systems, especially when it comes to a core-yet-commodity infrastructure 
service like samba, to warrant letting go of legacy.

Thanks for your good will and effort, nonetheless.

Bug#974868: samba-vfs-modules: Still causing issues - at least on armv5tel/armel

[Michael Tokarev]

17.11.2022 14:09, ArtMG wrote:
..

I have now reproduced the error on 4.13.13 on 32bit, and confirmed it is *still* an issue 
with 4.16.7 on 32bit. However when I switch to 64bit OS version, the error does *not* 
occur, not even in 4.13.13. The TimeMachine client successfully mounts the share and 
completes multiple backups, and the server smbd logs are clean, with no overflow 
warnings, and definitely no "Invalid or incomplete multibyte or wide character" 
errors.



Do you have ability to compile samba?  If yes (maybe with a debian package),
can you please try the attached patch?  It removes the useless and actually
wrong check for the off_t overflowing size_t.

Or alternatively I can provide pre-built binaries.

Thanks,

/mjtdiff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
index 4058d4834e7..8e31e74f2a6 100644
--- a/source3/modules/vfs_fruit.c
+++ b/source3/modules/vfs_fruit.c
@@ -5273,17 +5273,6 @@ static bool fruit_tmsize_do_dirent(vfs_handle_struct *handle,
 		return true;
 	}
 
-	/*
-	 * Arithmetic on 32-bit systems may cause overflow, depending on
-	 * size_t precision. First we check its unlikely, then we
-	 * force the precision into target off_t, then we check that
-	 * the total did not overflow either.
-	 */
-	if (bandsize > SIZE_MAX/nbands) {
-		DBG_ERR("tmsize potential overflow: bandsize [%zu] nbands [%zu]\n",
-			bandsize, nbands);
-		return false;
-	}
 	tm_size = (off_t)bandsize * (off_t)nbands;
 
 	if (state->total_size + tm_size < state->total_size) {

Bug#974868: samba-vfs-modules: Still causing issues - at least on armv5tel/armel

[Michael Tokarev]

17.11.2022 14:09, ArtMG wrote:

Please check if 4.16 fixes this. If not, I guess the best course is to
switch to a 64bit system, since apparently samba is having inherent
issues on 32bit systems.


I have now reproduced the error on 4.13.13 on 32bit, and confirmed it is *still* an issue 
with 4.16.7 on 32bit. However when I switch to 64bit OS version, the error does *not* 
occur, not even in 4.13.13. The TimeMachine client successfully mounts the share and 
completes multiple backups, and the server smbd logs are clean, with no overflow 
warnings, and definitely no "Invalid or incomplete multibyte or wide character" 
errors.


I looked at the source of vfs_fruit module.
It looks to me this whole thing is just trivial really.

First, they use system off_t type for internal thing.  It is
not used for actual system functions which expect an offset in
a file, it is used internally and/or in the protocol, in particular
to report disk free space, - there, an off_t should not be used.
For this reason, something like u_int64_t (or u_int_least64_t)
type should be used instead of off_t with an unknown size.

And second, when samba is built, LFS (large file support) should
be enabled, so off_t should already be 64bits.

To me it looks like somewhat questionable programming habits.

For an uint type, something like ~((uint64_t)0) (I havn't done
much programming in some 10 years - something *like* that) should
give you the maximum value this type can ever hold, - that's
SIZE_MAX for you.

I'm not sure what this very test gives you in the first place.
the max size *should* be able to hold a 64bit integer.

FWIW.

/mjt

Bug#974868: samba-vfs-modules: Still causing issues - at least on armv5tel/armel

[ArtMG]

> Please check if 4.16 fixes this. If not, I guess the best course is to
> switch to a 64bit system, since apparently samba is having inherent
> issues on 32bit systems.

I have now reproduced the error on 4.13.13 on 32bit, and confirmed it is 
*still* an issue with 4.16.7 on 32bit. However when I switch to 64bit OS 
version, the error does *not* occur, not even in 4.13.13. The TimeMachine 
client successfully mounts the share and completes multiple backups, and the 
server smbd logs are clean, with no overflow warnings, and definitely no 
"Invalid or incomplete multibyte or wide character" errors.

> Can we at least retitle this as "... on 32bi platforms",
> since what you describe suggest that it's not specific
> to 32bits.

I concur, mjt, that the new bug title more accurately describes the issue, now

> I for one don't know what vfs_fruit *is*, to begin with. Just read briefly the
> manpage, -- well, it has quite some things, it seems, most of which is related
> to MacOS.  I don't have a MacOS machine.

So, yes, vfs_fruit (named to indicate something apple-like without TM issues) 
is a module that extends samba to provide macOS-related features - essentially 
to serve SMB in the way that a Mac would. That means to test any issues one 
would indeed need some kind of client Mac hardware or simulator available.

> If Samba is to have long-term 32 bit support someone needs to provide
> an upstream patch to run a docker build in 32 bit, ie a 32 bit
> userspace on 64 bit docker hosts. 

Thank you for the wider upstream, persepctive, Andrew. In this particular case, 
tests also required a terrabyte of storage on hand so that the volume 
calculations would overflow. In my domestic one-off tests it was easy to lay my 
hands on an old external HDD and a Raspberry Pi. However in a larger-team, 
keeping a virtual setup for regression testing, and including the mac client 
for testing vfs_fruit features – I can see how that is going to make for much 
tougher logistics.

I guess the question now, it whether this gets marked as 'won't fix', because 
it only occurs in these specific circumstances ("fruit:time machine max size" 
is set) and for 32 bit systems, or whether it needs logging upstream? The 
upstream development cost is likely to be low for this specific bug, but its 
the ongoing commitment to testing that could weigh more heavily.

Personally I'm happy to migrate to 64bit and leave all the overflows behind me, 
but I'll leave it to you higher-level project experts to make the important 
decision. Thanks for all your work.

ArtMG

Bug#974868: [Pkg-samba-maint] Bug#974868: samba-vfs-modules: Still causing issues - at least on armv5tel/armel

[Andrew Bartlett]

On Wed, 2022-11-16 at 18:29 +, ArtMG wrote:
> Hi,
> 
> > On Mon, 31 Oct 2022 14:19:24 +0300 Michael Tokarev 
> wrote:
> > > Lacking any further information, I'll close this bugreport as
> fixed in 4.16.
> 
> I have just built and tested against 4.16.7 and I'm afraid to say
> that the reported issue DOES still occur. 
> 
> 
> On Tue, 15 Nov 2022 11:03:02 +0300 Michael Tokarev 
> wrote:
> > If you think this is incorrect and the issue is still here with
> current version
> 
> > of samba, please feel free to reopen this bug report with any extra
> information
> > which might be helpful to identify the issue.
> 
> What kind of extra information might be helpful? I can pull
> diagnostics off my test rig if it helps.
> 
> 
> Part of me wants to get this issue resolved so that 32-bit systems
> can meet these use-cases. However, I know from working on the patch
> for that other bug (
> https://bugzilla.samba.org/show_bug.cgi?id=13622#c10) that you
> increase the precision in one part of the code, and sooner or later
> an issue arises in another. 
> 
> In the end, there's a limit how long it's pragmatic to play bug
> whack-a-mole like this. Since my previous issue I have upgraded to
> hardware that supports 64-bit, so I'm now off to validate whether
> your suggestion of upgrading OS architecture can make this issue
> magically disappear.

If Samba is to have long-term 32 bit support someone needs to provide
an upstream patch to run a docker build in 32 bit, ie a 32 bit
userspace on 64 bit docker hosts. 

This should be entirely possible, but web searches show up very little,
and even 32 bit VMs seem hard to find (Douglas specially built one
years ago for a xsltproc doc build issue, but it was a hastle on most
cloud providers).

Bonus points if that 32 bit host is actually FreeBSD (to catch that as
well) somehow running under docker via qemu yet still behaving as a
subprocess, but that's off topic here. 

Andrew,


-- 
Andrew Bartlett (he/him)   https://samba.org/~abartlet/
Samba Team Member (since 2001) https://samba.org
Samba Team Lead, Catalyst IT   https://catalyst.net.nz/services/samba

Samba Development and Support, Catalyst IT - Expert Open Source
Solutions

Bug#974868: samba-vfs-modules: Still causing issues - at least on armv5tel/armel

[Michael Tokarev]

Control: retitle -1 samba: can't serve size-limited Time Machine shares on 
32bit architectures
Control: tag -1 + confirmed upstream
Control: severity -1 minor

16.11.2022 21:29, ArtMG wrote:

Hi,

 > On Mon, 31 Oct 2022 14:19:24 +0300 Michael Tokarev mailto:m...@tls.msk.ru>> wrote:
 > > Lacking any further information, I'll close this bugreport as fixed in 
4.16.

I have just built and tested against 4.16.7 and I'm afraid to say that the 
reported issue *DOES* still occur.


Ok.

It's good I didn't actually close this bug report as I wanted to - I thought
about sending the email to nnn-done@bugs.d.o, but forgot about this -done part.
(I was dealing with old bugs, there are *lots* of them reported against samba
package, they're just sitting there for years without any action or attention,
so something has to be done with them anyway).



On Tue, 15 Nov 2022 11:03:02 +0300 Michael Tokarev mailto:m...@tls.msk.ru>> wrote:
 > If you think this is incorrect and the issue is still here with current 
version
 > of samba, please feel free to reopen this bug report with any extra 
information
 > which might be helpful to identify the issue.

What kind of extra information might be helpful? I can pull diagnostics off my 
test rig if it helps.


I for one don't know what vfs_fruit *is*, to begin with. Just read briefly the
manpage, -- well, it has quite some things, it seems, most of which is related
to MacOS.  I don't have a MacOS machine.

So basically, I know nothing about how to verify this.

And the thing is that I can't really do anything there.
It's best to be handled upstream anyway, I don't know
samba internals.  I can handle packaging issues, but
for the real code issues, especially the ones which I
can't even verify myself - I can only close the bug
reports so the reports wont stack and hide somethin
which I really can fix.

Can we at least retitle this as "... on 32bi platforms",
since what you describe suggest that it's not specific
to 32bits.


Part of me wants to get this issue resolved so that 32-bit systems can meet 
these use-cases.
However, I know from working on the patch for that other 
bug (https://bugzilla.samba.org/show_bug.cgi?id=13622#c10) that you increase the precision in 
one part of the code, and sooner or later an issue arises in another.


Umm. So we've hit some internal limitations with off_t size
here.  That's understandable. Just yesterday we talked with
Qemu folks - they just *detest* 32bit platforms, since it's
extremly difficult to map even the 32bit address space into
its own 32bit address space, they have to perform all kinds
of tricks which doesn't work anyway in the end, - it is just
can not be done.


In the end, there's a limit how long it's pragmatic to play bug whack-a-mole 
like this.
 Since my previous issue I have upgraded to hardware that 
supports 64-bit, so I'm now off to validate whether your suggestion of upgrading

OS architecture can make this issue magically disappear.


I see. Well, this is at least practical, I think.

Thank you very much for your input!

/mjt

Bug#974868: samba-vfs-modules: Still causing issues - at least on armv5tel/armel

[ArtMG]

Hi,

> On Mon, 31 Oct 2022 14:19:24 +0300 Michael Tokarev  wrote:
> > Lacking any further information, I'll close this bugreport as fixed in 4.16.

I have just built and tested against 4.16.7 and I'm afraid to say that the 
reported issue *DOES* still occur. 

On Tue, 15 Nov 2022 11:03:02 +0300 Michael Tokarev  wrote:
> If you think this is incorrect and the issue is still here with current 
> version
> of samba, please feel free to reopen this bug report with any extra 
> information
> which might be helpful to identify the issue.

What kind of extra information might be helpful? I can pull diagnostics off my 
test rig if it helps.

Part of me wants to get this issue resolved so that 32-bit systems can meet 
these use-cases. However, I know from working on the patch for that other bug 
(https://bugzilla.samba.org/show_bug.cgi?id=13622#c10) that you increase the 
precision in one part of the code, and sooner or later an issue arises in 
another. 

In the end, there's a limit how long it's pragmatic to play bug whack-a-mole 
like this. Since my previous issue I have upgraded to hardware that supports 
64-bit, so I'm now off to validate whether your suggestion of upgrading OS 
architecture can make this issue magically disappear.

Thanks
ArtMG

Bug#974868: samba-vfs-modules: Still causing issues - at least on armv5tel/armel

[Michael Tokarev]

Version: 2:4.16.0+dfsg-1

On Mon, 31 Oct 2022 14:19:24 +0300 Michael Tokarev  wrote:
..

Lacking any further information, I'll close this bugreport as fixed in 4.16.


Let's close it now, it looks like there's no point to keep this bug report open.
If you think this is incorrect and the issue is still here with current version
of samba, please feel free to reopen this bug report with any extra information
which might be helpful to identify the issue.

Thanks,

/mjt

Bug#974868: samba-vfs-modules: Still causing issues - at least on armv5tel/armel

[Michael Tokarev]

Control: tag -1 + moreinfo

On Fri, 8 Apr 2022 09:11:27 +0300 Michael Tokarev  wrote:

Control: tag -1 - patch

On Tue, 15 Feb 2022 17:54:04 +0100 Daniel Gassen  wrote:
> Package: samba-vfs-modules
> Version: 2:4.13.13+dfsg-1~deb11u3
> Followup-For: Bug #974868
> X-Debbugs-Cc: dan...@gassen.io
> 
> Dear Maintainer,
> 
> unfortunately this bug doesn't seem to be fixed.


In that case this is some other issue, not the one mentioned in this bug
(https://bugzilla.samba.org/show_bug.cgi?id=13622).

Removing the tag (patch).

Other than that, I don't have any good news to share, unfortunately.

Please check if 4.16 fixes this. If not, I guess the best course is to
switch to a 64bit system, since apparently samba is having inherent
issues on 32bit systems. See #1006935 for another issue which appears
to be 32bit-related too - http://bugs.debian.org/1006935 .


The 32bit issues mentioned has been due to another, unrelated problem.

But the question still persist: does this still occur with current
version of samba, say, 4.16 from bullseye-backports?

Lacking any further information, I'll close this bugreport as fixed in 4.16.

Thanks,

/mjt

Bug#974868: samba-vfs-modules: Still causing issues - at least on armv5tel/armel

[Michael Tokarev]

Control: tag -1 - patch

On Tue, 15 Feb 2022 17:54:04 +0100 Daniel Gassen  wrote:

Package: samba-vfs-modules
Version: 2:4.13.13+dfsg-1~deb11u3
Followup-For: Bug #974868
X-Debbugs-Cc: dan...@gassen.io

Dear Maintainer,

unfortunately this bug doesn't seem to be fixed.


In that case this is some other issue, not the one mentioned in this bug
(https://bugzilla.samba.org/show_bug.cgi?id=13622).

Removing the tag (patch).

Other than that, I don't have any good news to share, unfortunately.

Please check if 4.16 fixes this. If not, I guess the best course is to
switch to a 64bit system, since apparently samba is having inherent
issues on 32bit systems. See #1006935 for another issue which appears
to be 32bit-related too - http://bugs.debian.org/1006935 .

Thanks,

/mjt