Saw this in 4.19.5, but given the code, I believe it also still happens in master. root@n-ad:~# samba-tool gpo manage motd set {177F310B-5416-48A9-8A39-EA8919EB7964} "Welcome" -U Administrator%Passw0rd WARNING: Using passwords on command line is insecure. Installing the setproctitle python module will hide these from shortly after program start. root@n-ad:~# samba-tool gpo manage motd set {177F310B-5416-48A9-8A39-EA8919EB7964} "Welcome" -U Administrator%Passw0rd WARNING: Using passwords on command line is insecure. Installing the setproctitle python module will hide these from shortly after program start. ERROR(<class 'UnboundLocalError'>): uncaught exception - cannot access local variable 'data' where it is not associated with a value File "/usr/lib/python3/dist-packages/samba/netcmd/__init__.py", line 279, in _run return self.run(*args, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/samba/netcmd/gpo.py", line 3829, in run text = ET.SubElement(data, 'text') ^^^^ try: xml_data = ET.fromstring(conn.loadfile(vgp_xml)) except NTSTATUSError as e: if e.args[0] in [NT_STATUS_OBJECT_NAME_INVALID, NT_STATUS_OBJECT_NAME_NOT_FOUND, NT_STATUS_OBJECT_PATH_NOT_FOUND]: # The file doesn't exist, so create the xml structure xml_data = ET.ElementTree(ET.Element('vgppolicy')) policysetting = ET.SubElement(xml_data.getroot(), 'policysetting') pv = ET.SubElement(policysetting, 'version') pv.text = '1' name = ET.SubElement(policysetting, 'name') name.text = 'Text File' description = ET.SubElement(policysetting, 'description') description.text = 'Represents a Generic Text File' apply_mode = ET.SubElement(policysetting, 'apply_mode') apply_mode.text = 'replace' data = ET.SubElement(policysetting, 'data') filename = ET.SubElement(data, 'filename') filename.text = 'motd' elif e.args[0] == NT_STATUS_ACCESS_DENIED: raise CommandError("The authenticated user does " "not have sufficient privileges") else: raise text = ET.SubElement(data, 'text') When the file doesn't exist yet, it's created, and then we have "data" populated, and it can be used outside the try/except block. If, however, the policy file exists already, no exception is raised, and we jump straight to the "text" assignment, which then fails because "data" is not defined.
Would this be a fix? I copied it from other similar functions without too much testing: diff --git a/python/samba/netcmd/gpo.py b/python/samba/netcmd/gpo.py index 96fce917f0f..fe9b7caacb2 100644 --- a/python/samba/netcmd/gpo.py +++ b/python/samba/netcmd/gpo.py @@ -3808,7 +3808,9 @@ samba-tool gpo manage motd set {31B2F340-016D-11D2-945F-00C04FB984F9} "Message f return try: - xml_data = ET.fromstring(conn.loadfile(vgp_xml)) + xml_data = ET.ElementTree(ET.fromstring(conn.loadfile(vgp_xml))) + policysetting = xml_data.getroot().find('policysetting') + data = policysetting.find('data') except NTSTATUSError as e: if e.args[0] in [NT_STATUS_OBJECT_NAME_INVALID, NT_STATUS_OBJECT_NAME_NOT_FOUND,
Yeah, this seems to work, and be correct.
According to comment four of https://bugs.launchpad.net/bugs/2092308, the fix in comment 1 here is not quite right (it appends text rather than replaces).
Created attachment 18527 [details] fix crash when updating an MOTD GPO
> According to comment four of https://bugs.launchpad.net/bugs/2092308, the fix > in comment 1 here is not quite right (it appends text rather than replaces). You are right, I forgot to update the patch here. New version attached.
(In reply to Andreas Hasenack from comment #5) > New version attached. Thanks. If you are able to format the patch with a commit message using `git commit -s` and `git format-patch`, this will ease its path upstream. (I think you have already done the contributor paperwork at https://www.samba.org/samba/devel/copyright-policy.html).
> (I think you have already done the contributor paperwork at > https://www.samba.org/samba/devel/copyright-policy.html). I did, on May 19, 2022, I just located the email I sent, and its confirmation.
Created attachment 18530 [details] fix crash when updating an MOTD GPO
Found another bug, this patch now crashes when the GPO doesn't exist yet. Update coming, I'm just testing it again. It will look like this, if all goes well: --- a/python/samba/netcmd/gpo.py +++ b/python/samba/netcmd/gpo.py @@ -3808,7 +3808,9 @@ return try: - xml_data = ET.fromstring(conn.loadfile(vgp_xml)) + xml_data = ET.ElementTree(ET.fromstring(conn.loadfile(vgp_xml))) + policysetting = xml_data.getroot().find('policysetting') + data = policysetting.find('data') except NTSTATUSError as e: if e.args[0] in [NT_STATUS_OBJECT_NAME_INVALID, NT_STATUS_OBJECT_NAME_NOT_FOUND, @@ -3834,7 +3836,9 @@ else: raise - text = ET.SubElement(data, 'text') + text = data.find('text') + if not text: + text = ET.SubElement(data, 'text') text.text = value out = BytesIO()
Created attachment 18574 [details] python:netcmd:gpo: fix crash when updating an MOTD GPO New version of the patch, which also works when there is no existing GPO to update.
Unfortunately I suspect there could be more bugs lurking around in this area of GPOs...
(In reply to Andreas Hasenack from comment #11) > Unfortunately I suspect there could be more bugs lurking around in this area of GPOs... Yes. It is not the best. https://gitlab.com/samba-team/samba/-/merge_requests/4006/diffs?commit_id=16305ec2b0e2aa31889eb7d330ca74f37c7c2c03 is your commit with one change: text = data.find('text') - if not text: + if text is None: text = ET.SubElement(data, 'text') text.text = value because the craziness of Python's xml.etree is that a <text> element with no elements inside is viewed like an empty list, evaulating as false, so `text = data.find('text')` finds the first <text> element, so with <data><text> A </text></data> because '<text> A </text>' has no sub-elements and is thus false, we'd end up appending another <text>: <data><text> A </text><text> B </text></data> when we really want <data><text> B </text></data> (I think you mentioned this problem somewhere).
This bug was referenced in samba master: 969cb41e06247949c3992cab25e824795204e31e e87e20c04d90292e3a5caac8ea3105b16f948ed3
> because '<text> A </text>' has no sub-elements and is thus false, we'd end up > appending another <text>: Ugh, you are right. I introduced this, then I thought I fixed it, and I'm seeing it again in the last iteration, argh
> Unfortunately I suspect there could be more bugs lurking around in this area of > GPOs... Found another one: # Clearing the MOTD text root@p-gpo:~# samba-tool gpo manage motd set {31B2F340-016D-11D2-945F-00C04FB984F9} "" -U Administrator%Passw0rd WARNING: Using passwords on command line is insecure. Installing the setproctitle python module will hide these from shortly after program start. # Listing root@p-gpo:~# samba-tool gpo manage motd list '{31B2F340-016D-11D2-945F-00C04FB984F9}' ERROR(<class 'TypeError'>): uncaught exception - write() argument must be str, not None File "/usr/lib/python3/dist-packages/samba/netcmd/__init__.py", line 353, in _run return self.run(*args, **kwargs) ~~~~~~~~^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/samba/netcmd/gpo.py", line 3751, in run self.outf.write(text.text) ~~~~~~~~~~~~~~~^^^^^^^^^^^ root@p-gpo:~# Something like this should fix it I hope? --- python/samba/netcmd/gpo.py.py.orig 2025-04-15 19:35:07.051084668 +0000 +++ python/samba/netcmd/gpo.py.py 2025-04-15 19:35:08.665997025 +0000 @@ -3748,7 +3748,9 @@ policy = xml_data.find('policysetting') data = policy.find('data') text = data.find('text') - self.outf.write(text.text) + if text is not None: + if text.text is not None: + self.outf.write(text.text) class cmd_set_motd(GPOCommand): """Sets a VGP MOTD Group Policy to the sysvol
I filed https://bugzilla.samba.org/show_bug.cgi?id=15848 for that last issue, let's not mix things.
This bug was referenced in samba v4-22-test: f252b2b42d86c3e539b0f9942ccfddffebd0bdef 7f76df8a6f02497c3f04622c694f9e2761a84797
This bug was referenced in samba v4-21-test: d1993a4a0e1575fbcc15787831b1955fe617bb82 d56d0122642560784840e4351c60ee112e2058c3
This bug was referenced in samba v4-22-stable (Release samba-4.22.1): f252b2b42d86c3e539b0f9942ccfddffebd0bdef 7f76df8a6f02497c3f04622c694f9e2761a84797