Bug 1198463 - samba script update-apparmor-samba-profile not working due to sed error
samba script update-apparmor-samba-profile not working due to sed error
Status: RESOLVED FIXED
Classification: openSUSE
Product: openSUSE Tumbleweed
Classification: openSUSE
Component: Samba
Current
Other Other
: P5 - None : Normal (vote)
: ---
Assigned To: Noel Power
The 'Opening Windows to a Wider World' guys
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2022-04-13 15:31 UTC by Noel Power
Modified: 2022-08-11 16:30 UTC (History)
3 users (show)

See Also:
Found By: ---
Services Priority:
Business Priority:
Blocker: ---
Marketing QA Status: ---
IT Deployment: ---


Attachments
patch for update-apparmor-samba-profile (2.41 KB, patch)
2022-04-13 19:52 UTC, Christian Boltz
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noel Power 2022-04-13 15:31:46 UTC
when update-apparmor-samba-profile runs (on smb start) the file /etc/apparmor.d/local/usr.sbin.smbd-shares is not updated (if fact it will be empty)

caused by sed error

sed: -e expression #1, char 42: delimiter character is not a single-byte character
Comment 1 Christian Boltz 2022-04-13 19:51:24 UTC
Looks like '§' the script uses as separator is no longer is a single-byte character, so we'll need to find another special character ;-)
Looking at the smiley - why not ';'?

While on it, let me add
- set -o pipefail
- || verboseexit "generating profile sniplet failed"   (after the sed call)

During testing, I also noticed that
    test -e "$profilesniplet" || \
    silentexit "apparmor profile snippet not available"
might not be a good idea, and replaced it with
    test -e "$profilesniplet" || touch "$profilesniplet"

I'll attach a patch ;-)
Comment 2 Christian Boltz 2022-04-13 19:52:04 UTC
Created attachment 858132 [details]
patch for update-apparmor-samba-profile
Comment 3 Noel Power 2022-04-13 22:34:08 UTC
(In reply to Noel Power from comment #0)
> when update-apparmor-samba-profile runs (on smb start) the file
> /etc/apparmor.d/local/usr.sbin.smbd-shares is not updated (if fact it will
> be empty)
> 
> caused by sed error
> 
> sed: -e expression #1, char 42: delimiter character is not a single-byte
> character

so the change here happened because the samba CI tests include a test that scans the source code to ensure it is utf8 compliant. The previous delimiter was a printable character with some other encoding most likley latin-8 (because it must have been a single byte) we changed that but didn't double check that sed still worked :-(

We discussed it here and decided that maybe a non printable delimiter is probably a better choice (and avoids mostly any chance that it could be part of a path name scanned by sed) so we chose the first single byte ascii char that worked e.g. 001

I am sure the other changes you have here are fine, but I'll look in more detail tomorrow (it might be we could integrate them in a second pass given some of of the team involved in discussing this just went on vacation)
Comment 4 Noel Power 2022-04-13 22:35:40 UTC
(In reply to Noel Power from comment #3)
> (In reply to Noel Power from comment #0)
> > when update-apparmor-samba-profile runs (on smb start) the file
> > /etc/apparmor.d/local/usr.sbin.smbd-shares is not updated (if fact it will
> > be empty)
> > 
> > caused by sed error
> > 
> > sed: -e expression #1, char 42: delimiter character is not a single-byte
> > character
> 
> so the change here happened because the samba CI tests include a test that
> scans the source code to ensure it is utf8 compliant. The previous delimiter
> was a printable character with some other encoding most likley latin-8
> (because it must have been a single byte) we changed that but didn't double
> check that sed still worked :-(
latin-1 (it's late for me)
Comment 5 Christian Boltz 2022-04-14 20:12:54 UTC
(In reply to Noel Power from comment #3)
> We discussed it here and decided that maybe a non printable delimiter is
> probably a better choice (and avoids mostly any chance that it could be part
> of a path name scanned by sed) so we chose the first single byte ascii char
> that worked e.g. 001

That's an interesting delimiter ;-) which I never used, and that might make the script a bit more interesting to read. (And I won't even think about copy&paste ;-)

To make things a bit easier - the delimiter should be avoided (or escaped) in the sed command (the 's/foo/bar/'), but it shouldn't be a problem if it appears in the input data (in this case the path name). This also means using ';' as delimiter (as my patch does) should work without problems.

If you want a quick confirmation of the above:
    echo 'test;delimiter' | sed 's;t;b;'
Comment 6 Noel Power 2022-04-15 11:31:29 UTC
(In reply to Christian Boltz from comment #5)
> (In reply to Noel Power from comment #3)
> > We discussed it here and decided that maybe a non printable delimiter is
> > probably a better choice (and avoids mostly any chance that it could be part
> > of a path name scanned by sed) so we chose the first single byte ascii char
> > that worked e.g. 001
> 
> That's an interesting delimiter ;-) which I never used, and that might make
> the script a bit more interesting to read.
most editors (at least gedit, emacs & vi) show it as a special char
> (And I won't even think about copy&paste ;-)
that might be a problem (but not really necessary and in editor cut/paste should work) :-)
> 
> To make things a bit easier - the delimiter should be avoided (or escaped)
> in the sed command (the 's/foo/bar/'), but it shouldn't be a problem if it
> appears in the input data (in this case the path name). This also means
> using ';' as delimiter (as my patch does) should work without problems.
> 
> If you want a quick confirmation of the above:
>     echo 'test;delimiter' | sed 's;t;b;'

right, I wonder why the special character was chosen as a delimiter in the first place, we thought it was intentionally chosen to ensure it was very unlikely to be in the input data.

We already have a submission with the non-printable char (as this was the reviewed change and we wanted to get this fix out asap)
I doubt anyone here has any religious objections to changing the delimiter :-) especially if it appearing in the input data doesn't have downside so we will try to update this again later
Comment 7 Christian Boltz 2022-04-15 16:15:52 UTC
(In reply to Noel Power from comment #6)
> right, I wonder why the special character was chosen as a delimiter in the
> first place, we thought it was intentionally chosen to ensure it was very
> unlikely to be in the input data.

Hehe, you can blame me for using '§' ;-)

When I wrote the script, the idea was simply "use something that is not '/' so that I don't have to escape the / in the replacement". No idea why I picked '§', maybe I thought '¿' or '¡' (also valid delimiters in latin1 times) would be a bit too exotic ;-)
Comment 8 Noel Power 2022-08-11 16:30:19 UTC
released in tw, marked as fixed