Re: [opensource-dev] Review Request: VWR-24333: Hardening against use of getLindenUserDir() before logging in.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/91/#review386 --- indra/llvfs/lldir.cpp http://codereview.secondlife.com/r/91/#comment272 I know we already have a lot of methods taking boolean arguments, but it's probably worth mentioning http://doc.qt.nokia.com/qq/qq13-apis.html#thebooleanparametertrap here. (I.e., boolean arguments make the calls of the method harder to read, when the method name doesn't already imply the semantics of the argument. You can give the reader a hint towards the semantics by using nicely named enums instead.) indra/newview/llappviewer.cpp http://codereview.secondlife.com/r/91/#comment273 E.g. here, it's impossible to guess what the true indicates. Either you already know, or you have to look it up. - Boroondas On Jan. 14, 2011, 1:05 p.m., Aleric Inglewood wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/91/ --- (Updated Jan. 14, 2011, 1:05 p.m.) Review request for Viewer. Summary --- Without this patch, getLindenUserDir() is sometimes used without checking if it returns an empty value, resulting in trying to open an empty file and then gracefully recovering from that. So, this patch doesn't really fix a bug. However it might prevent one in the future. Note that this DID lead to a bug in Viewer 1 code, so it's possible. The main threat is that some singleton class that uses getLindenUserDir (indirectly) is instantiated before logging in: A singleton is only created once and when it is initialized with an empty getLindenUserDir() then that can remain. This patch aborts when the viewer is compiled in debug mode (not in release mode, when it will do what it did before this patch) and when getLindenUserDir() is called before it was initialized, unless the developer explicitely passes 'true' (empty_ok) to getLindenUserDir, signaling that they considered the possibility that the function is being called before the user logged in. This addresses bug VWR-24333. http://jira.secondlife.com/browse/VWR-24333 Diffs - indra/llvfs/lldir.h 422f636c3343 indra/llvfs/lldir.cpp 422f636c3343 indra/newview/llappviewer.cpp 422f636c3343 indra/newview/llbottomtray.cpp 422f636c3343 indra/newview/llfilepicker.cpp 422f636c3343 indra/newview/llnavigationbar.cpp 422f636c3343 indra/newview/llsearchhistory.h 422f636c3343 indra/newview/llurlhistory.cpp 422f636c3343 indra/newview/llviewerinventory.cpp 422f636c3343 indra/newview/llviewermedia.cpp 422f636c3343 indra/newview/llvoiceclient.cpp 422f636c3343 Diff: http://codereview.secondlife.com/r/91/diff Testing --- Thanks, Aleric ___ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
Re: [opensource-dev] Review Request: VWR-24333: Hardening against use of getLindenUserDir() before logging in.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/91/#review190 --- Ship it! I haven't search for all instances of getLindenUserDir() in the whole code but, reading the diff, this all looks good to me. - Merov On Jan. 14, 2011, 1:05 p.m., Aleric Inglewood wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/91/ --- (Updated Jan. 14, 2011, 1:05 p.m.) Review request for Viewer. Summary --- Without this patch, getLindenUserDir() is sometimes used without checking if it returns an empty value, resulting in trying to open an empty file and then gracefully recovering from that. So, this patch doesn't really fix a bug. However it might prevent one in the future. Note that this DID lead to a bug in Viewer 1 code, so it's possible. The main threat is that some singleton class that uses getLindenUserDir (indirectly) is instantiated before logging in: A singleton is only created once and when it is initialized with an empty getLindenUserDir() then that can remain. This patch aborts when the viewer is compiled in debug mode (not in release mode, when it will do what it did before this patch) and when getLindenUserDir() is called before it was initialized, unless the developer explicitely passes 'true' (empty_ok) to getLindenUserDir, signaling that they considered the possibility that the function is being called before the user logged in. This addresses bug VWR-24333. http://jira.secondlife.com/browse/VWR-24333 Diffs - indra/llvfs/lldir.h 422f636c3343 indra/llvfs/lldir.cpp 422f636c3343 indra/newview/llappviewer.cpp 422f636c3343 indra/newview/llbottomtray.cpp 422f636c3343 indra/newview/llfilepicker.cpp 422f636c3343 indra/newview/llnavigationbar.cpp 422f636c3343 indra/newview/llsearchhistory.h 422f636c3343 indra/newview/llurlhistory.cpp 422f636c3343 indra/newview/llviewerinventory.cpp 422f636c3343 indra/newview/llviewermedia.cpp 422f636c3343 indra/newview/llvoiceclient.cpp 422f636c3343 Diff: http://codereview.secondlife.com/r/91/diff Testing --- Thanks, Aleric ___ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
[opensource-dev] Review Request: VWR-24333: Hardening against use of getLindenUserDir() before logging in.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/91/ --- Review request for Viewer. Summary --- Without this patch, getLindenUserDir() is sometimes used without checking if it returns an empty value, resulting in trying to open an empty file and then gracefully recovering from that. So, this patch doesn't really fix a bug. However it might prevent one in the future. Note that this DID lead to a bug in Viewer 1 code, so it's possible. The main threat is that some singleton class that uses getLindenUserDir (indirectly) is instantiated before logging in: A singleton is only created once and when it is initialized with an empty getLindenUserDir() then that can remain. This patch aborts when the viewer is compiled in debug mode (not in release mode, when it will do what it did before this patch) and when getLindenUserDir() is called before it was initialized, unless the developer explicitely passes 'true' (empty_ok) to getLindenUserDir, signaling that they considered the possibility that the function is being called before the user logged in. This addresses bug VWR-24333. http://jira.secondlife.com/browse/VWR-24333 Diffs - indra/llvfs/lldir.h 422f636c3343 indra/llvfs/lldir.cpp 422f636c3343 indra/newview/llappviewer.cpp 422f636c3343 indra/newview/llbottomtray.cpp 422f636c3343 indra/newview/llfilepicker.cpp 422f636c3343 indra/newview/llnavigationbar.cpp 422f636c3343 indra/newview/llsearchhistory.h 422f636c3343 indra/newview/llurlhistory.cpp 422f636c3343 indra/newview/llviewerinventory.cpp 422f636c3343 indra/newview/llviewermedia.cpp 422f636c3343 indra/newview/llvoiceclient.cpp 422f636c3343 Diff: http://codereview.secondlife.com/r/91/diff Testing --- Thanks, Aleric ___ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
