[Windows] Dynamic Security Plugin stores changes in dynsec.json.new instead of dynsec.json (affected: >= 2.0.16).

[lss4]

OS: Windows 10 22H2
Mosquitto Version: 2.0.18

Additional notes:

• I did not install Mosquitto to Program Files folder. I installed it to folder on a separate partition.
• Mosquitto is started through Services. SYSTEM does have permissions to Mosquitto's installation folder and files inside it.
• I'm using the user account created during Windows install, so UAC is active.

On my system, when I made changes using mosquitto_ctrl, changes are not directly applied to dynsec.json. Instead, a dynsec.json.new file is created and all changes are stored there while original dynsec.json remain unchanged.

This leads to a similar problem as #2938, as changes will be reverted on restart. Additionally, as I started Mosquitto as a service, dynsec.json.new would be created and owned by SYSTEM which means my own user cannot read nor modify it unless I added necessary permissions.

Further looking at the source code I found out it's storing the changes this way:

• It first creates a .new file to store the changes.
• It then renames the .new file to the original file.

Since there would be an error message should the rename fail I enabled error/warning logs and after trying to make a change, I immediately got the following line in the log file.

Error updating dynsec config file: File exists

I wonder if something in Win10 is getting in the way. On the other hand, I've another Windows system on which everything is working correctly (changes are correctly applied to the original file):
OS: Windows Server 2008 R2
Mosquitto Version: 2.0.10
Additional notes:

• On this system Mosquitto is installed in Program Files.
• Mosquitto is started through Services.
• Using built-in Administrator account thus there's no UAC or anything.

[imlarry]

I had the same issue and have confirmed that dynsec is working in V2.0.10 but fails in 2.0.18. For me, under 2.0.18 on Windows 10, I was unable to start the service if plugin_opt_config_file was defined and pointing at a "mosquitto_ctrl dynsec init" file. If it wasn't then the service started but log all has a warning that no config file was specified so the feature wouldn't do anything

Also, in mosquitto.conf there is no entry or documentation comments for the plugin_opt_config_file setting.

[lss4]

I don't know. When I looked at older binaries I noticed something.

The installer file size changed significantly at two spots: 2.0.9a/2.0.11 and 2.0.16. Version 2.0.10 was the last one to maintain a small filesize (about 1.6MB), and afterwards (somehow also 2.0.9a) the installer grew nearly tenfold to about 14MB (with 2.0.13-14 being 16MB). Since 2.0.16 the filesize grew even further to about 26MB.

I wonder if the changes in size during these spots were due to compiler changes. There's a portability issue with rename(), is that Windows does have a concept of rename which should fail if destination exists (hence my error), while Linux does not (it's the same as moving a file).

Probably older compilers handled the behavior in a portable way so it worked as desired for both Windows and Linux in this case (replacing the target file), but newer ones honored the intended behaviors for rename() on each OS causing the breakage on Windows builds.

[lss4]

A bump on this as I did tests on different versions. This is on a same system with Win10 22H2.

2.0.15: LGTM. dynsec.json file successfully updated. There's no dynsec.json.new in the folder.
2.0.16: Issue reproduced. dynsec.json not updated and dynsec.json.new appeared. File exists error reported in the log.

So the issue was not about Windows but the compiler/runtime changes happened during the period. I suppose if someone compiles 2.0.18 with the environment used to compile 2.0.15 the issue would be fixed without changing a single line of related code.

I wonder if the changes in size during these spots were due to compiler changes. There's a portability issue with rename(), is that Windows does have a concept of rename which should fail if destination exists (hence my error), while Linux does not (it's the same as moving a file).

Considering the breaking change (rename() fails when destination file exists) is the supposedly intended behavior on Windows, the code will definitely need to be refactored if one insists on using the latest compiler/runtime.

I've tried setting up a build environment for mosquitto but am so far unsuccessful. For now I'll be downgrading the affected system's mosquitto (which is 2.0.18) to 2.0.15 to work the issue around.

[imlarry]

ChangeLog.txt at ln 52 calls out a fix committed for 2.16 to address a race condition associated with mosquitto_ctrl init ... here's what changed.

3ab0a9a

The code invokes mosquitto_fopen() in the WIN32 conditional block which does the .new create and rename around ln 615 in plugins/dynamic-security/plugin.c

[lss4]

ChangeLog.txt at ln 52 calls out a fix committed for 2.16 to address a race condition associated with mosquitto_ctrl init ... here's what changed.

3ab0a9a

The code invokes mosquitto_fopen() in the WIN32 conditional block which does the .new create and rename around ln 615 in plugins/dynamic-security/plugin.c

Does this affect only mosquitto_ctrl dynsec init, or it affects any file operation involving dynsec.json? The changelog seems to suggest only that particular command (which generates a default config file).

But the reality is, all operations that involve modifying dynsec.json are failing as rename() errored due to "File exists". These operations have nothing to do with mosquitto_ctrl as they are done done via messages in respective MQTT topics.

[imlarry]

I encountered it in mosquitto_ctrl and not sure what else may be involved. I'm a SQL geek- using a flat file this way seems like a strategy to avoid locks on the readers but finding/fixing the underlying issue is beyond me.

[lss4]

I encountered it in mosquitto_ctrl and not sure what else may be involved. I'm a SQL geek- using a flat file this way seems like a strategy to avoid locks on the readers but finding/fixing the underlying issue is beyond me.

Looks like the code changed a lot during the period... I was kinda confused... although the line causing the error in the current version is still apparent.

I think the issue might be corrected if you remove() the destination file before rename() on Windows, though from the link I referenced, a race condition exists in this portable method, and the correct way would be to use the non-portable MoveFileEx().

[NorbertHeusser]

The main reason for usage of rename is to ensure the existence of a valid dynsec config file under all circumstance (as far as this is possble from a pure application development). The posix compliant rename by definition guarantees the existing new file will be untouched, if anything in the rename goes wrong.
If windows implementation is NOT posix compliant best would be to add an #ifdef/#else windows special implementation and replace the function by the proposed MoveFileEx using the MOVEFILE_REPLACE_EXISTING flag.
As I donÄt have a windows build machine available right now I cannot test a windows specific patch even on build errors.

[ir2000]

@NorbertHeusser the portable behaviour seems a much more obvious solution for any compiler or os

(void)remove(dest_file);
 
if (rename(src_file, dest_file) != 0) {
  /* Handle error condition */
}