Bugzilla – Bug 3231
nvault crashes server when writting to *.journal file w/o write perm
Last modified: 2014-02-08 23:52:19 PST
I noticed that if a .journal (and probaly .vault too) file doesn't have proper 'write' perms and the a write is attempted, it segfaults the server. It doesn't even give an error message. It appears internally there is no handling done for when a vault file cannot be opened for writing. There is no reason for there to be a crash. A notice in the logs should be enough. Any sys op should be able to check for such when they notice a plugin that uses nvault isn't behaving right.
Created attachment 3409 [details] [review] Fix NULL dereferences This patch fixes a couple of cases for Journal and NVault where NULL dereferencing can easily occur because of file permissions. In Journal::End() there is possible execution of fclose(NULL) which results in undefined behavior and most likely a segfault. NVault::Close() incorrectly assumes that m_Journal is not NULL, which it can be if we don't get a file handle beforehand. It is the only function to omit this check.
Comment on attachment 3409 [details] [review] Fix NULL dereferences Thanks for fixing this. >diff -r 19d71c1cf99c dlls/nvault/Journal.cpp >--- a/dlls/nvault/Journal.cpp Tue Jun 25 19:06:11 2013 +0200 >+++ b/dlls/nvault/Journal.cpp Mon Jul 01 20:01:02 2013 +0200 >@@ -123,13 +123,20 @@ > bool Journal::Begin() > { > m_fp = fopen(m_File.c_str(), "wb"); >- m_Bw.SetFilePtr(m_fp); >- return (m_fp != NULL); >+ if (m_fp) >+ { >+ m_Bw.SetFilePtr(m_fp); >+ return true; >+ } This change isn't needed, since BinaryWriter initializes m_fp.
(In reply to comment #2) > This change isn't needed, since BinaryWriter initializes m_fp. Are you sure it's not needed? That patch seems to be exactly what was missing; checking the return value of fopen to see if it actually succeeded /before/ passing it off to m.Bw.SetfilePtr(), as that's what appears to have causing the original problem.
SetFilePtr just sets m_fp, which is already NULL.
Yeah that particular change can safely be reverted without affecting this fix. I probably just thought it to be cleaner that way. The NULL dereferences are the real issue.
Created attachment 3413 [details] [review] Proposed fixed patch without unnecessary change Path looks good. Fix tested and prevents well a crash.
Pushed changeset: http://hg.alliedmods.net/amxmodx-central/rev/b883da1336eb Changelog: Fix server crash when nvault is writting to journal file without write permission (bug 3231, r=Arkshine)
Commit pushed to master at https://github.com/alliedmodders/amxmodx https://github.com/alliedmodders/amxmodx/commit/15572e2d61d4439ca120533c356513bd45818d33 Fix server crash when nvault is writting to journal file without write permission (bug 3231, r=Arkshine) Former-commit-id: 6ad9e6bc8cf748bb06d5618cb21741d6e79b0110