Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chocolatey.config gets corrupted when multiple processes access simultaneously #1047

Closed
mwallner opened this issue Nov 11, 2016 · 8 comments
Closed

Comments

@mwallner
Copy link
Member

First of all, I do know this problem seems to be related to ChocolateyGUI (read on) - but I think the error is in chocolatey.dll - so I decided to post this Issue here.

chocolatey.config xml gets corrupted

When starting chocolateyGUI while another process uses choco (like choco.exe from command-line) - chocolatey.config will be corrupted and can't be read any longer. - invalid xml, the XML serializer just seems to stop writing the file.

example of .config before the issue occurs

<?xml version="1.0" encoding="utf-8"?>
<chocolatey xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <containsLegacyPackageInstalls>false</containsLegacyPackageInstalls>
  <commandExecutionTimeoutSeconds>0</commandExecutionTimeoutSeconds>
  <config>
    <add key="cacheLocation" value="" description="Cache location if not TEMP folder." />
    <add key="containsLegacyPackageInstalls" value="true" description="Install has packages installed prior to 0.9.9 series." />
    <add key="commandExecutionTimeoutSeconds" value="2700" description="Default timeout for command execution." />
    <add key="proxy" value="" description="Explicit proxy location." />
    <add key="proxyUser" value="" description="Optional proxy user." />
    <add key="proxyPassword" value="" description="Optional proxy password. Encrypted." />
    <add key="webRequestTimeoutSeconds" value="30" description="Default timeout for web requests. Available in 0.9.10+." />
  </config>
  <sources>
    <source id="private-server" value="https://................../" disabled="false" priority="1" />
  </sources>
  <features>
    <feature name="checksumFiles" enabled="true" setExplicitly="false" description="Checksum files when pulled in from internet (based on package)." />
    <feature name="autoUninstaller" enabled="true" setExplicitly="false" description="Uninstall from programs and features without requiring an explicit uninstall script." />
    <feature name="allowGlobalConfirmation" enabled="false" setExplicitly="false" description="Prompt for confirmation in scripts or bypass." />
    <feature name="failOnAutoUninstaller" enabled="false" setExplicitly="false" description="Fail if automatic uninstaller fails." />
    <feature name="failOnStandardError" enabled="false" setExplicitly="false" description="Fail if install provider writes to stderr. Available in 0.9.10+." />
    <feature name="powershellHost" enabled="true" setExplicitly="false" description="Use Chocolatey's built-in PowerShell host. Available in 0.9.10+." />
    <feature name="logEnvironmentValues" enabled="false" setExplicitly="false" description="Log Environment Values - will log values of environment before and after install (could disclose sensitive data). Available in 0.9.10+." />
    <feature name="virusCheck" enabled="false" setExplicitly="false" description="Virus Check - perform virus checking on downloaded files. Available in 0.9.10+. Licensed versions only." />
    <feature name="failOnInvalidOrMissingLicense" enabled="false" setExplicitly="false" description="Fail On Invalid Or Missing License - allows knowing when a license is expired or not applied to a machine. Available in 0.9.10+." />
    <feature name="ignoreInvalidOptionsSwitches" enabled="true" setExplicitly="false" description="Ignore Invalid Options/Switches - If a switch or option is passed that is not recognized, should choco fail? Available in 0.9.10+." />
    <feature name="usePackageExitCodes" enabled="true" setExplicitly="false" description="Use Package Exit Codes - Package scripts can provide exit codes. With this on, package exit codes will be what choco uses for exit when non-zero (this value can come from a dependency package). Chocolatey defines valid exit codes as 0, 1605, 1614, 1641, 3010. With this feature off, choco will exit with a 0 or a 1 (matching previous behavior). Available in 0.9.10+." />
    <feature name="useFipsCompliantChecksums" enabled="false" setExplicitly="false" description="Use FIPS Compliant Checksums - Ensure checksumming done by choco uses FIPS compliant algorithms. Not recommended unless required by FIPS Mode. Enabling on an existing installation could have unintended consequences related to upgrades/uninstalls. Available in 0.9.10+." />
    <feature name="allowEmptyChecksums" enabled="true" setExplicitly="true" description="Allow packages to have empty/missing checksums for downloaded resources from non-secure locations (HTTP, FTP). Enabling is not recommended if using sources that download resources from the internet. Available in 0.10.0+." />
    <feature name="allowEmptyChecksumsSecure" enabled="true" setExplicitly="false" description="Allow packages to have empty/missing checksums for downloaded resources from secure locations (HTTPS). Available in 0.10.0+." />
    <feature name="scriptsCheckLastExitCode" enabled="false" setExplicitly="false" description="Scripts Check $LastExitCode (external commands) - Leave this off unless you absolutely need it while you fix your package scripts  to use `throw 'error message'` or `Set-PowerShellExitCode #` instead of `exit #`. This behavior started in 0.9.10 and produced hard to find bugs. If the last external process exits successfully but with an exit code of not zero, this could cause hard to detect package failures. Available in 0.10.3+. Will be removed in 0.11.0." />
  </features>
  <apiKeys />
</chocolatey>

example of .config afterwards

<?xml version="1.0" encoding="utf-8"?>
<chocolatey xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <containsLegacyPackageInstalls>false</containsLegacyPackageInstalls>
  <commandExecutionTimeoutSeconds>0</commandExecutionTimeoutSeconds>
  <config>
    <add key="cacheLocation" value="" description="Cache location if not TEMP folder." />
    <add key="containsLegacyPackageInstalls" value="true" description="Install has packages installed prior to 0.9.9 series." />
    <add key="commandExecutionTimeoutSeconds" value="2700" description="Default timeout for command execution." />
    <add key="proxy" value="" description="Explicit proxy location." />
    <add key="proxyUser" value="" description="Optional proxy user." />
    <add key="proxyPassword" value="" description="Optional proxy password. Encrypted." />
    <add key="webRequestTimeoutSeconds" value="30" description="Default timeout for web requests. Available in 0.9.10+." />
  </config>
  <sources>
    <source id="private-server" value="https://................../" disabled="false" priority="1" />
  </sources>
  <features>
    <feature name="checksumFiles" enabled="true" setExplicitly="false" description="Checksum files when pulled in from internet (based on package)." />
    <feature name="autoUninstaller" enabled="true" setExplicitly="false" description="Uninstall from programs and features without requiring an explicit uninstall script." />
    <feature name="allowGlobalConfirmation" enabled="false" setExplicitly="false" description="Prompt for confirmation in scripts or bypass." />
    <feature name="failOnAutoUninstaller" enabled="false" setExplicitly="false" description="Fail if automatic uninstaller fails." />
    <feature name="failOnStandardError" enabled="false" setExplicitly="false" description="Fail if install provider writes to stderr." />
    <feature name="powershellHost" enabled="true" setExplicitly="false" description="Use Chocolatey's built-in PowerShell host." />
    <feature name="logEnvironmentValues" enabled="false" setExplicitly="false" description="Log Environment Values - will log values of environment before and after install (could disclose sensitive data)." />
    <feature name="virusCheck" enabled="false" setExplicitly="false" description="Virus Check [licensed versions only] - perform virus checking on downloaded files." />
    <feature name="failOnInvalidOrMissingLicense" enabled="false" setExplicitly="false" description="Fail On Invalid Or Missing License - allows knowing when a license is expired or not applied to a machine." />
    <feature name="ignoreInvalidOptionsSwitches" enabled="true" setExplicitly="false" description="Ignore Invalid Options/Switches - If a switch or option is passed that is not recognized, should choco fail?" />
    <feature name="usePackageExitCodes" enabled="true" setExplicitly="false" description="Use Package Exit Codes - Package scripts can provide exit codes. With this on, package exit codes will be what choco uses for exit when non-zero (this value can come from a dependency package). Chocolatey defines valid exit codes as 0, 1605, 1614, 1641, 3010. With this feature off, choco will exit with a 0 or a 1 (matching previous behavior). Available in 0.9.10+." />
    <feature name="useFipsCompliantChecksums" enabled="false" setExplicitly="false" description="Use FIPS Compliant Checksums - Ensure checksumming done by choco uses FIPS compliant algorithms. Not recommended unless required by FIPS Mode. Enabling on an existing installation could have unintended consequences related to upgrades/uninstalls. Available in 0.9.10+." />
    <feature name="allowEmptyChecksums" enabled="true" setExplicitly="true" description="Allow packages to have empty/missing checksums for downloaded resources from non-secure locations (HTTP, FTP). Enabling is not recommended if using sources that download resources from the internet. Available in 0.10.0+." />
    <feature name="allowEmptyChecksumsSecure" enabled="true" setExplicitly="false" description="Allow packages to have empty/missing checksums for downloaded resources from secure locations (HTTPS). Available in 0.10.0+." />
    <feature name="scriptsCheckLastExitCode" enabled="false" setExplicitly="false" description="Scripts Check $LastExitCode (external commands) - Leave this off unless you absolutely need it while you fix your package scripts  to use `throw 

once this occurs, chocolatey can't be used anymore:

the output choco list -lo -verbose -debug :

Error deserializing response of type chocolatey.infrastructure.app.configuration.ConfigFileSettings:
 There is an error in XML document (32, 242).
Chocolatey had an error occur:
System.InvalidOperationException: There is an error in XML document (32, 242). ---> System.Xml.XmlException: There is an unclosed literal string. Line 32, position 242.
   at System.Xml.XmlTextReaderImpl.Throw(Exception e)
   at System.Xml.XmlTextReaderImpl.ParseAttributeValueSlow(Int32 curPos, Char quoteChar, NodeData attr)
   at System.Xml.XmlTextReaderImpl.ParseAttributes()
   at System.Xml.XmlTextReaderImpl.ParseElement()
   at System.Xml.XmlTextReaderImpl.ParseElementContent()
   at System.Xml.XmlReader.MoveToContent()
   at Microsoft.Xml.Serialization.GeneratedAssembly.XmlSerializationReaderConfigFileSettings.Read6_ConfigFileSettings(Boolean isNullable, Boolean checkType)
   at Microsoft.Xml.Serialization.GeneratedAssembly.XmlSerializationReaderConfigFileSettings.Read7_chocolatey()
   --- End of inner exception stack trace ---
   at System.Xml.Serialization.XmlSerializer.Deserialize(XmlReader xmlReader, String encodingStyle, XmlDeserializationEvents events)
   at chocolatey.infrastructure.services.XmlService.<>c__DisplayClass1`1.<deserialize>b__0()
   at chocolatey.infrastructure.tolerance.FaultTolerance.try_catch_with_logging_exception[T](Func`1 function, String errorMessage, Boolean throwError, Boolean logWarningInsteadOfError, Boolean logDebugInsteadOfError, Boolean isSilent)
   at chocolatey.infrastructure.services.XmlService.deserialize[XmlType](String xmlFilePath)
   at chocolatey.infrastructure.app.builders.ConfigurationBuilder.set_up_configuration(IList`1 args, ChocolateyConfiguration config, Container container, ChocolateyLicense license, Action`1 notifyWarnLoggingAction)
   at chocolatey.console.Program.Main(String[] args)

chocolatey.config should only be read, not written every time ChocolateySources are read via chocolatey.dll

Why does chocolatey.config need to be written every time ChocolateyGUI is opened?

steps to reproduce

  • start choco.exe from command-line (make sure it will take a while - install a large package or an update ..)
  • start ChocolateyGUI - et'voila C:\ProgramData\chocolatey\config\chocolatey.config will be corrupted

screenshots

choco_bug

Having a look at the soures of chocolateyGUI reveals that this happens during initialization of chocolatey.dll
choco_bug_get_sources

@mwallner
Copy link
Member Author

The error seems to be in chocolatey/infrastructure/services/XmlService.cs
I can reproduce the _fileSystem.copy_file to be ?unsuccessful? (only a certain amount of data is replaced within chocolatey.config) - afterwards xmlUpdateFilePath gets deleted.
Maybe a timing issue? (xmlUpdateFilePath should only be deleted after _fileSystem.copy_file has finished..)

image

@ferventcoder
Copy link
Member

@mwallner note that this writes a file called chocolatey.config.update, then it compares this file versus the original to see if it has changed (line 86 in your image). Only then does it copy over the existing.

I think possibly we need to read the file back in to see if it is valid prior to overwriting. What do you think?

@mwallner
Copy link
Member Author

@ferventcoder as far as I've tracked the bug it seems to be a timing issue .. I think the file isn't really written synchronously with _fileSystem.copy_file - and _fileSytem.delete_file stops the copy-operation somewhere during it's runtime..

I'd suggest to only delete the .update file if the content of the .config file and the .update file match

RichiCoder1 added a commit to RichiCoder1/choco that referenced this issue Dec 10, 2016
Rewrites the XML serialization service in a few specific ways:
- Should update hash checks now builds the update file in memory rather than committing it the disk, and checks the hash in memory.
- The ".update" file now using the slightly hardier File Replace to replace the undated file, and also generates a ".backup".
- When desieralizing, we now check for a ".backup" if serialization fails and attempt that too.
- Generally use streams and dispose where possible.
RichiCoder1 added a commit to RichiCoder1/choco that referenced this issue Dec 10, 2016
Rewrites the XML serialization service in a few specific ways:
- Should update hash checks now builds the update file in memory rather than committing it the disk, and checks the hash in memory.
- The ".update" file now using the slightly hardier File Replace to replace the undated file, and also generates a ".backup".
- When desieralizing, we now check for a ".backup" if serialization fails and attempt that too.
- Generally use streams and dispose where possible.
RichiCoder1 added a commit to RichiCoder1/choco that referenced this issue Dec 20, 2016
Allows for the more correct replacement and backup of files on the filesystem.
RichiCoder1 added a commit to RichiCoder1/choco that referenced this issue Dec 20, 2016
RichiCoder1 added a commit to RichiCoder1/choco that referenced this issue Dec 20, 2016
Allows for the more correct replacement and backup of files on the filesystem.
RichiCoder1 added a commit to RichiCoder1/choco that referenced this issue Dec 20, 2016
@ferventcoder ferventcoder self-assigned this Feb 7, 2017
RichiCoder1 added a commit to RichiCoder1/choco that referenced this issue Feb 8, 2017
Allows for the more correct replacement and backup of files on the filesystem.
RichiCoder1 added a commit to RichiCoder1/choco that referenced this issue Feb 8, 2017
RichiCoder1 added a commit to RichiCoder1/choco that referenced this issue Feb 8, 2017
@ferventcoder ferventcoder changed the title chocolatey.config gets corrupted when multiple processes access chcolatey.dll simultaneously chocolatey.config gets corrupted when multiple processes access simultaneously Feb 27, 2017
ferventcoder pushed a commit that referenced this issue Mar 23, 2017
Allows for the more correct replacement and backup of files on the filesystem.
ferventcoder pushed a commit that referenced this issue Mar 23, 2017
Previously, the xml file serialization could cause corruption when it
did not properly replace the file or when multiple choco processes were
running at the same time. Additionally, there was no backup of the
original config, so the configuration would need to be reset back to
the default configuration and then readjusted.

Switch to evaluating differences in memory instead of an update file.
If the upates are different, then write out the update file and use
replace file to have it create a backup of the old file and replace the
config with the updated version.

When deserializing, if the file is corrupt, look to use the backup and
put it back in place if it works. Handle cases where there is no original
config file and the backup file doesn't exist.

Another benefit of comparing in memory is that no errors are logged for
non-admins when the config update file can't be written.
ferventcoder added a commit that referenced this issue Mar 23, 2017
* pr1084:
  (GH-1047) Robust/efficient xml serialization
  (GH-1047) Add "replace" filesystem operation
  (GH-1047) Add additional hash stream options to Providers
ferventcoder added a commit that referenced this issue Mar 23, 2017
* stable:
  (GH-1047) Robust/efficient xml serialization
  (GH-1047) Add "replace" filesystem operation
  (GH-1047) Add additional hash stream options to Providers
@ferventcoder
Copy link
Member

Fixed for 0.10.4

@mwallner
Copy link
Member Author

Please reopen, still an Issue in 0.10.5

here's a sample of chocolatey.config after two processes accessed it simultaneously:

<?xml version="1.0" encoding="utf-8"?>
<chocolatey xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <containsLegacyPackageInstalls>false</containsLegacyPackageInstalls>
  <commandExecutionTimeoutSeconds>0</commandExecutionTimeoutSeconds>
  <config>
    <add key="cacheLocation" value="" description="Cache location if not TEMP folder." />
    <add key="containsLegacyPackageInstalls" value="true" description="Install has packages installed prior to 0.9.9 series." />
    <add key="commandExecutionTimeoutSeconds" value="2700" description="Default timeout for command execution." />
    <add key="

1 good thing though: now there's a .config.backup and config.update file as well - still containing the correct configuration.

@tek0011
Copy link

tek0011 commented Apr 19, 2017

Isnt it re-referenced here?

#1241

oh nope nevermind. this ticket is linked to that, but closed. I do still have this issue in 10.5 as well. We just moved all our hosts back to 10.3 for the time being.

@ferventcoder
Copy link
Member

The fixes that went into for this are supposed to look at the file after it saves it and determine if it is good - if not it should roll back old config.

@ferventcoder
Copy link
Member

Let's open a new issue and reference this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants