Hi Zoran and Anders, Thanks for your comments. Will send V2 patch for review with your proposals & suggestions.
Regards, Vu > -----Original Message----- > From: Zoran Milinkovic [mailto:zoran.milinko...@ericsson.com] > Sent: Friday, January 26, 2018 1:28 PM > To: anders.bjornerst...@telia.com; Vu Minh Nguyen > <vu.m.ngu...@dektech.com.au>; ravisekhar.ko...@oracle.com > Cc: opensaf-devel@lists.sourceforge.net > Subject: RE: [devel] [PATCH 0/1] Review Request for imm: not allow creating > reserved IMM class names [#2771] > > Hi, > > I agree with you that the main check must be on the service side. > > I've been thinking of adding a parameter in immnd.conf to define reserved > class names like: > export > RESERVED_CLASSES=classes,objects,attr_dflt,attrdef,multi_value_int,etc.... > > This will apply as well with the coming change in IMM where any database > can be used for PBE. > So, if someone wants to use another database, they may design database > tables in different ways, and may have different reserved class names. > > The default reserved class names should be from the default SQLite database > that are used today. > > BR, > Zoran > > -----Original Message----- > From: and...@acm.org [mailto:anders.bjornerst...@telia.com] > Sent: den 26 januari 2018 13:05 > To: Vu Minh Nguyen <vu.m.ngu...@dektech.com.au>; Zoran Milinkovic > <zoran.milinko...@ericsson.com>; ravisekhar.ko...@oracle.com > Cc: opensaf-devel@lists.sourceforge.net > Subject: Re: [devel] [PATCH 0/1] Review Request for imm: not allow creating > reserved IMM class names [#2771] > > Hi Vu, > > If the check is for preventing a "fatal" problem, (such as the PBE *never* > coming back up, resulting presumably sooner or later in arollback to the > latest > backup), thenI argue the check/prevention needs to at least be in the server. > It vould of course be both in the server and the library, but usually it is > better > to avoid redundant checks since it simplifies testing of such checks. > > The risk with having the checks done only at the client/library side is that > the > server is left vulnerable to "bad" clients/applications. > OpenSAF has no control over the application software. Application software > may corrupt its own memory resulting in the sending of corrupt messages. You > could even contemplate malicious attacks exploiting such a known > vulnerability if they manage to get access to the client side of the services. > > Thus I think the server side should be made "bullet proof" as far as is > known/possible in protecting itself expecially against faults of the maximum > gravity, i.e. causing cyclickal restarts and where not even a cluster restart > helps. You would have to go for a disruptive restore from backup here as I > understand it. > > As always when a new *serious* problem like this is discovered I expect that a > regression test case will be added to the imm testing suites somewhere and to > the system tests. > > Regards > Anders Bjornerstedt > > > > > >----Ursprungligt meddelande---- > >Från : vu.m.ngu...@dektech.com.au > >Datum : 2018-01-26 - 11:33 (GMT) > >Till : anders.bjornerst...@telia.com, ravisekhar.ko...@oracle.com, > >zoran.milinko...@ericsson.com Kopia : > >opensaf-devel@lists.sourceforge.net > >Ämne : Re: [devel] [PATCH 0/1] Review Request for imm: not allow > >creating reserved IMM class names [#2771] > > > >Hi Anders Bjornerstedt, > > > >Do you mean we should move the validity check to IMMND (e.g: fevs local > check) or have additional check at IMMND side? > > > >I thought the check here is about validation of user's input parameters, then > it could be better to detect invalid ones as early as possible. > > > >Regards, Vu > > > >> -----Original Message----- > >> From: and...@acm.org [mailto:anders.bjornerst...@telia.com] > >> Sent: Thursday, January 25, 2018 5:32 PM > >> To: vu.m.ngu...@dektech.com.au; ravisekhar.ko...@oracle.com; > >> zoran.milinko...@ericsson.com > >> Cc: opensaf-devel@lists.sourceforge.net > >> Subject: Re: [devel] [PATCH 0/1] Review Request for imm: not allow > >> creating reserved IMM class names [#2771] > >> > >> Hi, > >> > >> I think the general principle needs to be that validity checks to > >> eliminate the risk of introducing database inconsistency needs to be > >> performed in the server. The server needs to protect itself from any > >> faulty client generating "fatal" messages. > >> > >> Thanks > >> Anders Bjornerstedt > >> > >> > >> > >> > >> >----Ursprungligt meddelande---- > >> >Från : vu.m.ngu...@dektech.com.au > >> >Datum : 2018-01-25 - 13:54 (GMT) > >> >Till : zoran.milinko...@ericsson.com, ravisekhar.ko...@oracle.com > >> >Kopia : opensaf-devel@lists.sourceforge.net > >> >Ämne : [devel] [PATCH 0/1] Review Request for imm: not allow > >> >creating > >> reserved IMM class names [#2771] > >> > > >> >Summary: imm: not allow creating reserved IMM class names [#2771] > >> >Review request for Ticket(s): 2771 Peer Reviewer(s): Zoran, Ravi > >> >Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE *** > >> >Affected branch(es): develop, release Development branch: > >> >ticket-2771 Base revision: c1daa9cc8e583d0a6024b28241f2b671bfa615d8 > >> >Personal repository: git://git.code.sf.net/u/winhvu/review > >> > > >> >-------------------------------- > >> >Impacted area Impact y/n > >> >-------------------------------- > >> > Docs n > >> > Build system n > >> > RPM/packaging n > >> > Configuration files n > >> > Startup scripts n > >> > SAF services y > >> > OpenSAF services n > >> > Core libraries n > >> > Samples n > >> > Tests n > >> > Other n > >> > > >> > > >> >Comments (indicate scope for each "y" above): > >> >--------------------------------------------- > >> >*** EXPLAIN/COMMENT THE PATCH SERIES HERE *** > >> > > >> >revision 8e072e9858c105266d7975ad366a579b720dfcae > >> >Author:Vu Minh Nguyen <vu.m.ngu...@dektech.com.au> > >> >Date:Thu, 25 Jan 2018 20:40:08 +0700 > >> > > >> >imm: not allow creating reserved IMM class names [#2771] > >> > > >> >PBE will be restarted and will never come up if user requests > >> >creating IMM class object class which is reserved by PBE. > >> > > >> >This patch adds code to reject such request with > >> SA_AIS_ERR_INVALID_PARAM. > >> > > >> > > >> > > >> >Complete diffstat: > >> >------------------ > >> > src/imm/agent/imma_om_api.cc | 27 ++++++++++++++++++++++++++- > >> > 1 file changed, 26 insertions(+), 1 deletion(-) > >> > > >> > > >> >Testing Commands: > >> >----------------- > >> >Create class, same name as reserved classes. E.g: "objects" > >> > > >> > > >> >Testing, Expected Results: > >> >-------------------------- > >> >Get SA_AIS_ERR_INVALID_PARAM and PBE is not restarted > >> > > >> > > >> >Conditions of Submission: > >> >------------------------- > >> >Get ack from peer reviewers > >> > > >> > > >> >Arch Built Started Linux distro > >> >------------------------------------------- > >> >mips n n > >> >mips64 n n > >> >x86 n n > >> >x86_64 n n > >> >powerpc n n > >> >powerpc64 n n > >> > > >> > > >> >Reviewer Checklist: > >> >------------------- > >> >[Submitters: make sure that your review doesn't trigger any > >> >checkmarks!] > >> > > >> > > >> >Your checkin has not passed review because (see checked entries): > >> > > >> >___ Your RR template is generally incomplete; it has too many blank > entries > >> > that need proper data filled in. > >> > > >> >___ You have failed to nominate the proper persons for review and push. > >> > > >> >___ Your patches do not have proper short+long header > >> > > >> >___ You have grammar/spelling in your header that is unacceptable. > >> > > >> >___ You have exceeded a sensible line length in your > >> headers/comments/text. > >> > > >> >___ You have failed to put in a proper Trac Ticket # into your commits. > >> > > >> >___ You have incorrectly put/left internal data in your comments/files > >> > (i.e. internal bug tracking tool IDs, product names etc) > >> > > >> >___ You have not given any evidence of testing beyond basic build tests. > >> > Demonstrate some level of runtime or other sanity testing. > >> > > >> >___ You have ^M present in some of your files. These have to be > removed. > >> > > >> >___ You have needlessly changed whitespace or added whitespace crimes > >> > like trailing spaces, or spaces before tabs. > >> > > >> >___ You have mixed real technical changes with whitespace and other > >> > cosmetic code cleanup changes. These have to be separate commits. > >> > > >> >___ You need to refactor your submission into logical chunks; there is > >> > too much content into a single commit. > >> > > >> >___ You have extraneous garbage in your review (merge commits etc) > >> > > >> >___ You have giant attachments which should never have been sent; > >> > Instead you should place your content in a public tree to be pulled. > >> > > >> >___ You have too many commits attached to an e-mail; resend as > threaded > >> > commits, or place in a public tree for a pull. > >> > > >> >___ You have resent this content multiple times without a clear indication > >> > of what has changed between each re-send. > >> > > >> >___ You have failed to adequately and individually address all of the > >> > comments and change requests that were proposed in the initial > review. > >> > > >> >___ You have a misconfigured ~/.gitconfig file (i.e. user.name, > >> >user.email > >> etc) > >> > > >> >___ Your computer have a badly configured date and time; confusing the > >> > the threaded patch review. > >> > > >> >___ Your changes affect IPC mechanism, and you don't present any results > >> > for in-service upgradability test. > >> > > >> >___ Your changes affect user manual and documentation, your patch > series > >> > do not contain the patch that updates the Doxygen manual. > >> > > >> > > >> >-------------------------------------------------------------------- > >> >---------- Check out the vibrant tech community on one of the > >> >world's most engaging tech sites, Slashdot.org! > >> >http://sdm.link/slashdot > >> >_______________________________________________ > >> >Opensaf-devel mailing list > >> >Opensaf-devel@lists.sourceforge.net > >> >https://lists.sourceforge.net/lists/listinfo/opensaf-devel > >> > > > > > > >----------------------------------------------------------------------- > >------- Check out the vibrant tech community on one of the world's most > >engaging tech sites, Slashdot.org! http://sdm.link/slashdot > >_______________________________________________ > >Opensaf-devel mailing list > >Opensaf-devel@lists.sourceforge.net > >https://lists.sourceforge.net/lists/listinfo/opensaf-devel > > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
