AzureAD / rms-sdk-for-cpp

RMS SDK for C++
MIT License
29 stars 15 forks source link

Create a new pull request for branch Feature/fileapi pdf protector #184

Closed FXKewang closed 8 months ago

FXKewang commented 6 years ago

As Vishal request, close the current PR and create a new one. The code review tool we use doesn't open the current PR since it has a lot of commits.

vishmittal commented 6 years ago

@vishmittal , as I have ever asked you that whether MSIPC SDK and MIP SDK need to create protected file without wrapper. Your answer was they don't need to. So it is designed to create a protected PDF using a wrapper always and it is not configurable.

In addition, I think the applicaiton layer (but the fileapi layer) should determine where the default PDF wrapper is.

sdk/rms_sdk/FileAPI/Protector.h, line 58 at r3 (raw file):

 * with the wrapper document. It extends the ability to set the wrapper document.
 */
class ProtectorWithWrapper : public Protector

Do we always create a protected PDF using a wrapper? Is that configurable? Also, we should have the default PDF wrapper as a resource checked in the source code.


Comments from Reviewable

vishmittal commented 6 years ago

Done. 6a71dac..995181e

sdk/rms_sdk/FileAPI/PDFProtector.cpp, line 315 at r3 (raw file):

  memcpy(reinterpret_cast<uint8_t *>(&vec_publishing_license[0]),
         publishing_license, publishing_license_size);
  //vec_publishing_license.push_back('\0');

remove any unused or commented out code


Comments from Reviewable

vishmittal commented 6 years ago

Done. b4bbc91..b1beec0

sdk/rms_sdk/UnitTests/fileapi_ut/pdf_protector_ut/PDFProtector_child.h, line 11 at r3 (raw file):

#ifndef RMSSDK_UNITTESTS_FILEAPI_UT_PDF_PROTECTOR_UT_PDFPROTECTOR_CHILD_H_
#define RMSSDK_UNITTESTS_FILEAPI_UT_PDF_PROTECTOR_UT_PDFPROTECTOR_CHILD_H_

This is an exact copy of PDFProtector.cpp and not a mock. Please delete it and add mock later when we create unit tests The end to end tests can use the actual PDFProtector.cpp


Comments from Reviewable

vishmittal commented 6 years ago

Done. 995181e..2a15610

sdk/rms_sdk/PDFObjectModel/pdf_module_mgr.cpp, line 25 at r3 (raw file):


PDFModuleMgrImpl::PDFModuleMgrImpl() {
  pdf_codec_module_ = CCodec_ModuleMgr::Create();

this constructor should be private since this is a singleton class.


Comments from Reviewable

vishmittal commented 6 years ago

Done. 2a15610..521e797

sdk/rms_sdk/PDFObjectModel/pdf_module_mgr.h, line 49 at r5 (raw file):

  std::shared_ptr<PDFSecurityHandler> shared_security_hander_;

  static PDFModuleMgrImpl* instance_;

You can do the singleton implementation very simply without 'instance_' as a member variable. You can do it like this:

static PDFModuleMgrImpl& Instance() { static PDFModuleMgrImpl instance; return instance; }

and then the bool PDFModuleMgr::Initialize() would look like this:

PDFModuleMgr::Initialize() { if (g_pdf_module_mgr == nullptr) { g_pdf_module_mgr.reset(&PDFModuleMgrImpl::Instance()); }

return true; }

Since PDFModuleMgrImpl is a Singleton, you should delete the copy and move constructors:

PDFModuleMgrImpl(const CPDFModuleMgrImpl&) = delete; PDFModuleMgrImpl& operator=(const CPDFModuleMgrImpl&) = delete; PDFModuleMgrImpl(CPDFModuleMgrImpl&&) = delete; PDFModuleMgrImpl& operator=(CPDFModuleMgrImpl&&) = delete;

Refer here: http://www.nuonsoft.com/blog/2017/08/10/implementing-a-thread-safe-singleton-with-c11-using-magic-statics/


Comments from Reviewable

vishmittal commented 6 years ago

Done. 87f2368..0be1a22 Other comments:

  1. CreateCustomerSecurityHandler(void* param), param is used to pass user-defined data.
  2. foxit pdf core takes over custom_security_handler, if use shared_ptr, it will be released twice.
  3. Yes, RMS_LIBRARY_STATIC and RMS_CALLING_CONVENTION are used for static build for MSIPC.

Reviewed 6 of 212 files at r1, 68 of 69 files at r2, 213 of 259 files at r3. Review status: 203 of 223 files reviewed at latest revision, 20 unresolved discussions.


sdk/rms_sdk/PDFObjectModel/pdf_creator.cpp, line 415 at r3 (raw file):

    const std::string& filter_name,
    const std::vector<unsigned char> &publishing_license,
    std::shared_ptr<PDFCryptoHandler> crypto_hander,

nit: typo 'handler'


sdk/rms_sdk/PDFObjectModel/pdf_creator.cpp, line 605 at r3 (raw file):

    CPDF_Parser *pdf_parser,
    CPDF_Dictionary* encryption_dictionary,
    std::shared_ptr<PDFCryptoHandler> crypto_hander,

nit: typo 'handler'


sdk/rms_sdk/PDFObjectModel/pdf_creator.cpp, line 640 at r3 (raw file):

    PDFSharedStream inputIOS,
    const std::string& filter_name,
    std::shared_ptr<PDFSecurityHandler> security_hander,

nit: typo 'handler'


sdk/rms_sdk/PDFObjectModel/pdf_module_mgr.cpp, line 25 at r3 (raw file):

Previously, vishmittal (Vishal Mittal) wrote…
this constructor should be private since this is a singleton class.

LGTM


sdk/rms_sdk/PDFObjectModel/pdf_module_mgr.cpp, line 48 at r5 (raw file):

}

void PDFModuleMgrImpl::SetSharedSecurityHandler(std::shared_ptr<PDFSecurityHandler> shared_security_hander) {

typo: shared_security_handler


sdk/rms_sdk/PDFObjectModel/pdf_module_mgr.cpp, line 50 at r5 (raw file):

void PDFModuleMgrImpl::SetSharedSecurityHandler(std::shared_ptr<PDFSecurityHandler> shared_security_hander) {
  if (shared_security_hander_) {
    shared_security_hander_.reset();

you only need to do either reset() or make it equal to nullptr. They are equivalent


sdk/rms_sdk/PDFObjectModel/pdf_module_mgr.cpp, line 59 at r5 (raw file):

}

static CPDF_SecurityHandler* CreateCustomerSecurityHandler(void* param) {

typo error 'CreateCustomSecurityHandler'


sdk/rms_sdk/PDFObjectModel/pdf_module_mgr.cpp, line 59 at r5 (raw file):

}

static CPDF_SecurityHandler* CreateCustomerSecurityHandler(void* param) {

Delete if 'param' is not being used anywhere


sdk/rms_sdk/PDFObjectModel/pdf_module_mgr.cpp, line 64 at r5 (raw file):

  std::shared_ptr<PDFSecurityHandler> security_hander = pdf_module_mgr->GetSharedSecurityHandler();
  //core takes over custom_security_handler
  CustomSecurityHandler* custom_security_handler = new CustomSecurityHandler(security_hander);

use smart pointer here. You can do:

auto custom_security_handler = std::make_shared(security_handler); return custom_security_handler.get();


sdk/rms_sdk/PDFObjectModel/pdf_module_mgr.cpp, line 68 at r5 (raw file):

}

void PDFModuleMgrImpl::RegisterSecurityHandler(const std::string& filter_name, std::shared_ptr<PDFSecurityHandler> security_hander) {

typo: security_handler


sdk/rms_sdk/PDFObjectModel/pdf_module_mgr.cpp, line 77 at r5 (raw file):

        CreateCustomerSecurityHandler,
        nullptr);
  }

you could call PDFModuleMgr::Initialize() if g_pdf_module_mgr is nullptr


sdk/rms_sdk/PDFObjectModel/pdf_module_mgr.cpp, line 89 at r5 (raw file):

      nullptr,
      nullptr);
}

Indentation


sdk/rms_sdk/PDFObjectModel/pdf_module_mgr.cpp, line 89 at r5 (raw file):

      nullptr,
      nullptr);
}

you could call PDFModuleMgr::Initialize() if g_pdf_module_mgr is nullptr


sdk/rms_sdk/PDFObjectModel/pdf_object_model.h, line 22 at r3 (raw file):

//#define CDECL_CALLING_CONVENTION

#ifdef RMS_LIBRARY_STATIC

are RMS_LIBRARY_STATIC and RMS_CALLING_CONVENTION used for static build for MSIPC?


Comments from Reviewable

vishmittal commented 6 years ago

Won't fix. Because foxit pdf core takes over custom_security_handler, if use shared_ptr, it will be released twice.

sdk/rms_sdk/PDFObjectModel/pdf_module_mgr.cpp, line 64 at r5 (raw file):

Previously, vishmittal (Vishal Mittal) wrote…
use smart pointer here. You can do: auto custom_security_handler = std::make_shared(security_handler); return custom_security_handler.get();

It should be: auto custom_security_handler = std::make_shared(security_handler); return custom_security_handler.get();


Comments from Reviewable

vishmittal commented 6 years ago

Done. ed181a5..cbf8d2d

Other comments:

  1. PDFWrapperDoc is used to parse PDF IRM V1&V2 files. PDFUnencryptedWrapperCreator is used to create PDF IRM V2 files.

Reviewed 3 of 259 files at r3. Review status: 205 of 223 files reviewed at latest revision, 25 unresolved discussions.


sdk/rms_sdk/FileAPI/PDFProtector.h, line 37 at r5 (raw file):

 * It is implemented by PDF protect, and it is invoked by PDF object model to access to the stream data.
 */
class FDFDataStreamImpl : public pdfobjectmodel::PDFDataStream {

should be 'PDFDataStreamImpl'


sdk/rms_sdk/PDFObjectModel/basic.cpp, line 133 at r3 (raw file):

  }
}

nit: remove extra empty line


sdk/rms_sdk/PDFObjectModel/pdf_object_model.h, line 126 at r5 (raw file):

 * @brief The type definitions of PDF wrapper doc.
 */
enum PDFWrapperDocType{

use scoped enums. https://blog.smartbear.com/programming-languages/closer-to-perfection-get-to-know-c11-scoped-and-based-enum-types/


sdk/rms_sdk/PDFObjectModel/pdf_object_model.h, line 141 at r5 (raw file):

 * It is implemented by PDF object model, and it is invoked by PDF protector.
 */
class PDFWrapperDoc {

It seems PDFWrapperDoc and PDFUnencryptedWrapperCreator are related in functionality. Why don't you have a single class for them?


sdk/rms_sdk/PDFObjectModel/pdf_object_model.h, line 332 at r5 (raw file):

 * @brief The error code definitions for PDFCreator.
 */
enum PDFCreatorErr{

use scoped enums. https://blog.smartbear.com/programming-languages/closer-to-perfection-get-to-know-c11-scoped-and-based-enum-types/


Comments from Reviewable

vishmittal commented 6 years ago

Done. cbf8d2d..e4aff45

sdk/rms_sdk/PDFObjectModel/unencrypted_wrapper.cpp, line 125 at r3 (raw file):

bool PDFWrapperDocImpl::StartGetPayload(PDFSharedStream output_stream) {
  if (wrapper_type_ == PDFWrapperDocType::IRMV1 || is_irmv2_without_wrapper) {
    uint8_t* buffer_pointer_temp = new uint8_t[payload_size_];

use vector instead of char*


Comments from Reviewable

vishmittal commented 6 years ago

Done. de442da..d3dff37 Other comments, 1.Yes. It is just to suppress the compiler warning.

  1. If the parse_result is not success, CreateUnencryptedWrapper will return false.

Reviewed 2 of 259 files at r3, 3 of 12 files at r6. Review status: 207 of 223 files reviewed at latest revision, 25 unresolved discussions.


sdk/rms_sdk/PDFObjectModel/pdf_creator.cpp, line 24 at r3 (raw file):

FX_BOOL CustomCryptoHandler::Init(CPDF_Dictionary* encryption_dictionary,
                                  CPDF_SecurityHandler* security_handler) {
  FX_UNREFERENCED_PARAMETER(encryption_dictionary);

is this just to suppress the compiler warning?


sdk/rms_sdk/PDFObjectModel/unencrypted_wrapper.cpp, line 182 at r3 (raw file):

  wrapper_file_stream_ = std::make_shared<FileStreamImpl>(wrapper_doc_stream);
  FX_DWORD parse_result = pdf_parser_.StartParse(wrapper_file_stream_.get());
  if (PDFPARSE_ERROR_SUCCESS == parse_result) {

What happens if the parse_result is not success?


sdk/rms_sdk/PDFObjectModel/unencrypted_wrapper.cpp, line 191 at r3 (raw file):

  pdf_parser_.CloseParser();

  wrapper_file_stream_.reset();

you need to do either reset() or assign a nullptr to it. No need to do both since they are equivalent.


sdk/rms_sdk/PDFObjectModel/unencrypted_wrapper.cpp, line 195 at r3 (raw file):


  if (payload_file_stream_) {
    payload_file_stream_.reset();

you need to do either reset() or assign a nullptr to it. No need to do both since they are equivalent.


sdk/rms_sdk/PDFObjectModel/unencrypted_wrapper.cpp, line 225 at r3 (raw file):


  if (payload_file_stream_) {
    payload_file_stream_.reset();

you need to do either reset() or assign a nullptr to it. No need to do both since they are equivalent.


Comments from Reviewable

vishmittal commented 6 years ago

Done. 8e1a99e..41ad2b6

Reviewed 4 of 12 files at r6. Review status: 211 of 223 files reviewed at latest revision, 31 unresolved discussions.


sdk/rms_sdk/FileAPI/PDFProtector.cpp, line 113 at r6 (raw file):

    pdfobjectmodel::PDFBinaryBuf* dest_buf) {
  FILEAPI_UNREFERENCED_PARAMETER(dest_buf);
  data_to_be_decrypted_->write(src_buf, src_size);

check if 'data_to_bedecrypted' is null first before writing to it


sdk/rms_sdk/FileAPI/PDFProtector.h, line 30 at r6 (raw file):

 */
#define MIN_RAW_SIZE 64 * 1024 * 1024
#define PROGRESSIVE_ENCRYPT_TEMP_FILE ".RMS.PE.temp"

append a random number to this temp file name so that multiple threads running in parallel don't create an issue. You can use a method that returns this temp file name.


sdk/rms_sdk/PDFObjectModel/pdf_creator.cpp, line 297 at r6 (raw file):


    publishing_license_bytestring.Empty();
    if (dest_buf[0] == 0xef && dest_buf[1] == 0xbb && dest_buf[2] == 0xbf) {

add comments about this encoding logic


sdk/rms_sdk/PDFObjectModel/pdf_creator.cpp, line 466 at r6 (raw file):

}

bool PDFCreatorImpl::IsDynamicXFA(CPDF_Parser *pdf_parser) {

Add comments about what this method does


sdk/rms_sdk/PDFObjectModel/pdf_creator.h, line 11 at r6 (raw file):


/**
 * @brief The implementaion class of interface class CPDF_CryptoHandler of Foxit core.

typo: implementation


sdk/rms_sdk/PDFObjectModel/pdf_creator.h, line 70 at r6 (raw file):


/**
 * @brief The implementaion class of interface class CPDF_ProgressiveEncryptHandler of Foxit core.

typo: implementation


Comments from Reviewable

vishmittal commented 6 years ago

Yes. PDF core will take over the dictionary created.

Reviewed 2 of 12 files at r6. Review status: 212 of 223 files reviewed at latest revision, 20 unresolved discussions.


sdk/rms_sdk/PDFObjectModel/pdf_creator.cpp, line 572 at r6 (raw file):

    const std::string& filter_name,
    const std::vector<unsigned char> &publishing_license) {
  CPDF_Dictionary* encryption_dictionary = new CPDF_Dictionary;

Does PDF core take over and release this memory? If not, use a smart pointer here


sdk/rms_sdk/PDFObjectModel/pdf_creator.cpp, line 576 at r6 (raw file):

  encryption_dictionary->SetAtName("Filter", filter_name.c_str());

  CPDF_Dictionary* pCFDic = new CPDF_Dictionary;

Does PDF core take over and release this memory? If not, use a smart pointer here


sdk/rms_sdk/PDFObjectModel/pdf_creator.cpp, line 578 at r6 (raw file):

  CPDF_Dictionary* pCFDic = new CPDF_Dictionary;

  CPDF_Dictionary* crypt_filter_dictionary = new CPDF_Dictionary;

Does PDF core take over and release this memory? If not, use a smart pointer here


Comments from Reviewable