Bug 3231 - nvault crashes server when writting to *.journal file w/o write perm
nvault crashes server when writting to *.journal file w/o write perm
Status: RESOLVED FIXED
Product: AMX Mod X
Classification: Unclassified
Component: Module: nVault
trunk
PC All
: P4 normal
Assigned To: amxmodx-bugs@alliedmods.net
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-04 00:15 PDT by blz
Modified: 2014-02-08 23:52 PST (History)
4 users (show)

See Also:
dvander: blocking‑1.8.2+


Attachments
Fix NULL dereferences (913 bytes, patch)
2013-07-01 11:16 PDT, Valentin Grünbacher [:Nextra]
no flags Details | Diff | Splinter Review
Proposed fixed patch without unnecessary change (708 bytes, patch)
2013-07-02 01:13 PDT, Vincent Herbet [:Arkshine]
hv.contact: review+
Details | Diff | Splinter Review

Description blz 2008-08-04 00:15:13 PDT
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.
Comment 1 Valentin Grünbacher [:Nextra] 2013-07-01 11:16:12 PDT
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 2 David Anderson [:dvander] 2013-07-01 11:22:15 PDT
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.
Comment 3 blz 2013-07-01 13:01:19 PDT
(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.
Comment 4 David Anderson [:dvander] 2013-07-01 13:03:55 PDT
SetFilePtr just sets m_fp, which is already NULL.
Comment 5 Valentin Grünbacher [:Nextra] 2013-07-01 16:19:13 PDT
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.
Comment 6 Vincent Herbet [:Arkshine] 2013-07-02 01:13:53 PDT
Created attachment 3413 [details] [review]
Proposed fixed patch without unnecessary change


Path looks good.
Fix tested and prevents well a crash.
Comment 7 AM Bugzilla Bot 2013-07-04 11:45:30 PDT
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)
Comment 8 AM Bugzilla Bot 2014-02-08 23:52:19 PST
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

Note You need to log in before you can comment on or make changes to this bug.