I think we can push this and continue reviewing / testing after it has
been pushed. Right now I have two review comments:
1)
I think there is an integer underflow bug in the code below:
+ size_t length_to_copy;
+ void *p;
+ if
(ntfHeader->additionalInfo[i].infoValue.ptrVal.dataSize
+ <
(osaf_extended_name_length(ptr) + 2)) {
+ LOG_ER("The allocated memory is
not large enough,"
+ " the object
will be truncated (%s)"
+ ,
osaf_extended_name_borrow(ptr));
+ length_to_copy =
ntfHeader->additionalInfo[i].infoValue.ptrVal.dataSize - 2;
+ } else
+ length_to_copy =
osaf_extended_name_length(ptr);
+ p = send_param->variable_data.p_base +
+ ntfHeader->additionalInfo[i].infoValue.ptrVal.dataOffset + 2;
+ memcpy(p,
osaf_extended_name_borrow(ptr), length_to_copy);
If ntfHeader->additionalInfo[i].infoValue.ptrVal.dataSize is less than
2, then length_to_copy can underflow and become very large.
2)
Why is ntfsv_dealloc_notification() called explicitly here, instead of
doing it in the destructor? Or rather, maybe I should ask: is
sendNotInfo really allocated using "new", and not using malloc() /
calloc()? In that case, we should call free() rather than "delete".
diff --git a/osaf/services/saf/ntfsv/ntfs/NtfAdmin.cc
b/osaf/services/saf/ntfsv/ntfs/NtfAdmin.cc
--- a/osaf/services/saf/ntfsv/ntfs/NtfAdmin.cc
+++ b/osaf/services/saf/ntfsv/ntfs/NtfAdmin.cc
@@ -301,6 +301,7 @@ void NtfAdmin::notificationReceivedUpdat
// we have got the notification
TRACE_2("notification %u received"
" again, skipped", (unsigned int)notificationId);
+ ntfsv_dealloc_notification(sendNotInfo);
delete sendNotInfo;
}
else
@@ -344,6 +345,8 @@ void NtfAdmin::notificationReceivedColdS
// we have got the notification
TRACE_2("notification %u received"
" again, skipped", (unsigned int)notificationId);
+ ntfsv_dealloc_notification(sendNotInfo);
+ delete sendNotInfo;
}
On 08/08/2014 08:58 AM, Minh Hon Chau wrote:
> Summary: NTF: Support DNs longer than 255 bytes [#873] V4
> Review request for Trac Ticket(s): [#873]
> Peer Reviewer(s): AndersW, Praveen, Mathi, Zoran, AndersBj
> Pull request to:
> Affected branch(es): default
> Development branch: default
>
> --------------------------------
> 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 y
> Other n
>
>
> Comments (indicate scope for each "y" above):
> ---------------------------------------------
> <<EXPLAIN/COMMENT THE PATCH SERIES HERE>>
>
> changeset 79eb61e69d36d54808a60190613be8e59ef8c6bf
> Author: Minh Hon Chau <minh.c...@dektech.com.au>
> Date: Fri, 08 Aug 2014 16:28:29 +1000
>
> NTF: Adapt NTF API to support long DNs [#873] V2
>
> (1) Fix longdn deallocation followed by Agent(or client) memory owner
>
> (2) Support SA_NTF_VALUE_LDAP_NAME in AdditionalInfo
>
> changeset d4d540ac3a1f1ad36340b9fd59ee2868e5a977a8
> Author: Minh Hon Chau <minh.c...@dektech.com.au>
> Date: Fri, 08 Aug 2014 16:28:29 +1000
>
> NTF: Adapt NTF common library to support long DNs [#873] V4
>
> (1) Fix longdn deallocaton followed by Agent(or client) memory owner
>
> (2) Remove ntfs_sanamet_strdup, ntfsv_sanamet_clone_strptr
>
> changeset b35b71c4ae7afc335c5782d921ab3f63e4df54e9
> Author: Minh Hon Chau <minh.c...@dektech.com.au>
> Date: Fri, 08 Aug 2014 16:28:29 +1000
>
> NTF: Adapt NTFIMCND to support long DNs [#873]
>
> changeset 75322a3cee57b1509c79022f01373d93dbcae205
> Author: Minh Hon Chau <minh.c...@dektech.com.au>
> Date: Fri, 08 Aug 2014 16:28:29 +1000
>
> NTF: Adapt NTF osaf service to support long DNs [#873] V2
>
> (1) Retry logging with truncated notificationObject/notifyingObject as
> no
> long dn support in LOG Service
>
> (2) Fix longdn deallocation followed by memory owner
>
> changeset e1e6b369e535c267d50347948e4fa88e84c74f30
> Author: Minh Hon Chau <minh.c...@dektech.com.au>
> Date: Fri, 08 Aug 2014 16:28:29 +1000
>
> Adapt NTF tools (ntfread, ntfsend, ntfsubscribe) to support long
> DNs[#873]
> V3
>
> (1) Remove ntfs_saname_strdup
>
> changeset 7ef7c1657350eba771572dcc1b6c0a73ce987edf
> Author: Minh Hon Chau <minh.c...@dektech.com.au>
> Date: Fri, 08 Aug 2014 16:28:29 +1000
>
> NTF: Add ntftest test cases for notification with long dn objects
> [#873] V3
>
> (1) Add testcase for AdditionInfo with extended name
>
> (2) Add testcase for create/modify/delete runtime&config object with
> extended name attribute
>
>
> Added Files:
> ------------
> tests/ntfsv/tet_longDnObject_notification.c
>
>
> Complete diffstat:
> ------------------
> osaf/libs/agents/saf/ntfa/Makefile.am | 1 +
> osaf/libs/agents/saf/ntfa/ntfa.h | 1 +
> osaf/libs/agents/saf/ntfa/ntfa_api.c | 53 +-
> osaf/libs/agents/saf/ntfa/ntfa_util.c | 30 +-
> osaf/libs/common/ntfsv/Makefile.am | 1 +
> osaf/libs/common/ntfsv/include/ntfsv_mem.h | 29 +-
> osaf/libs/common/ntfsv/ntfsv_enc_dec.c | 41 +-
> osaf/libs/common/ntfsv/ntfsv_mem.c | 198 +++++++--
> osaf/libs/saf/libSaNtf/Makefile.am | 1 +
> osaf/services/saf/ntfsv/ntfimcnd/Makefile.am | 2 +
> osaf/services/saf/ntfsv/ntfimcnd/ntfimcn_imm.c | 91 ++-
> osaf/services/saf/ntfsv/ntfimcnd/ntfimcn_notifier.c | 54 +-
> osaf/services/saf/ntfsv/ntfs/Makefile.am | 1 +
> osaf/services/saf/ntfsv/ntfs/NtfAdmin.cc | 3 +
> osaf/services/saf/ntfsv/ntfs/NtfFilter.cc | 44 +-
> osaf/services/saf/ntfsv/ntfs/NtfLogger.cc | 40 +-
> osaf/services/saf/ntfsv/ntfs/ntfs_evt.c | 7 +-
> osaf/services/saf/ntfsv/ntfs/ntfs_main.c | 5 +
> osaf/tools/safntf/include/ntfclient.h | 3 +-
> osaf/tools/safntf/ntfread/Makefile.am | 1 +
> osaf/tools/safntf/ntfread/ntfread.c | 23 +-
> osaf/tools/safntf/ntfsend/Makefile.am | 1 +
> osaf/tools/safntf/ntfsend/ntfsend.c | 29 +-
> osaf/tools/safntf/ntfsubscribe/Makefile.am | 2 +
> osaf/tools/safntf/ntfsubscribe/ntfsubscribe.c | 4 +
> osaf/tools/safntf/src/Makefile.am | 1 +
> osaf/tools/safntf/src/ntfclient.c | 54 +-
> tests/ntfsv/Makefile.am | 3 +-
> tests/ntfsv/test_ntf_imcn.c | 508
> +++++++++++++++++++++++-
> tests/ntfsv/tet_longDnObject_notification.c | 972
> ++++++++++++++++++++++++++++++++++++++++++++++
> tests/unit_test_fw/inc/util.h | 5 +-
> tests/unit_test_fw/src/Makefile.am | 1 +
> tests/unit_test_fw/src/util.c | 14 +-
> 33 files changed, 1958 insertions(+), 265 deletions(-)
>
>
> Testing Commands:
> -----------------
> ntftest
>
>
> Testing, Expected Results:
> --------------------------
> all tests must pass (plus suite 35 36)
>
>
> Conditions of Submission:
> -------------------------
> <<HOW MANY DAYS BEFORE PUSHING, CONSENSUS ETC>>
>
>
> Arch Built Started Linux distro
> -------------------------------------------
> mips n n
> mips64 n n
> x86 n n
> x86_64 y 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 ~/.hgrc file (i.e. username, 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.
>
------------------------------------------------------------------------------
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
