S
S
specific-blueberry2020-08-28 21:46:26
C++ / C#
specific-blueberry, 2020-08-28 21:46:26

What is the best way to manage memory?

I am writing a program to get data from Microsoft OLE2 files . I found a library with which you can pull out data. Everything is fine, everything works. But you need to improve the code, deal with leaks.
How it works?
Microsoft OLE2 files is a file system - there are directories and files (stream).
The program finds two files (/path/to/Data, /path/to/Property), reads them, extracts data, then they can be written to the database.

#include <iostream>
#include "com.h"
#include "utf.h"
#include <windows.h>
#include <cstdint>
#include <algorithm>
#include <sstream>
#include <iomanip>
#include <vector> 
#pragma execution_character_set( "utf-8" )

const CFB::COMPOUND_FILE_ENTRY* FindStream(const CFB::CompoundFileReader& reader, const char* streamName) {
    const CFB::COMPOUND_FILE_ENTRY* ret = nullptr;

    reader.EnumFiles(reader.GetRootEntry(), -1,
        [&](const CFB::COMPOUND_FILE_ENTRY* entry, const CFB::utf16string& u16dir, int level)->void {

            if (reader.IsStream(entry)) {
                std::string name = UTF16ToUTF8(entry->name);
                
                if (u16dir.length() > 0) {
                
                    std::string dirName = UTF16ToUTF8(u16dir.c_str(), u16dir.length());
                    std::replace(dirName.begin(), dirName.end(), '\n', '\\');
                    dirName.erase(std::remove(dirName.begin(), dirName.end(), (char) 0x00), dirName.end());
                    std::string fullName = dirName + '\\' + name;
                    if (strcmp(fullName.c_str(), streamName) == 0) {
                        ret = entry;
                    }
                }
                else {
                    if (strcmp(streamName, name.c_str()) == 0)
                    {
                        ret = entry;
                    }
                }
            }
        }
    );
    return ret;
}

char* ReadStream(const CFB::CompoundFileReader& reader, const CFB::COMPOUND_FILE_ENTRY* entry) {
    char* buf = new char[entry->size];
    reader.ReadFile(entry, 0, buf, entry->size);
    return buf;
}

void ExtractFromDataStream(const CFB::CompoundFileReader& reader, const CFB::COMPOUND_FILE_ENTRY* entry) {
    char* buf = ReadStream(reader, entry);
    ...
    delete[] buf;
}

// returns some struct, not void.
void ExtractFromPropertyStream(const CFB::CompoundFileReader& reader, const CFB::COMPOUND_FILE_ENTRY* entry) {
    char* buf = ReadStream(reader, entry);
    ...
    delete[] buf;
}

const CFB::CompoundFileReader GetReader(std::string path) {
    FILE* fp;
    fopen_s(&fp, path.c_str(), "rb");
    if (fp == NULL) {
        std::cerr << "read file error" << std::endl;
    }

    fseek(fp, 0, SEEK_END);
    size_t len = ftell(fp);
    unsigned char* buffer = new unsigned char[len];
    fseek(fp, 0, SEEK_SET);
    len = fread(buffer, 1, len, fp);
    fclose(fp);
    CFB::CompoundFileReader reader(buffer, len);
    return reader;
}

int main()
{
    SetConsoleCP(1251);
    SetConsoleOutputCP(1251);
    std::string path = "path\\to\\file.dat";
    auto reader = GetReader(path);
    auto dataStream = FindStream(reader, "path\\to\\Data");
    if (dataStream == nullptr) {
        std::cerr << "dataStream is null" << std::endl;
        return 1;
    }
    auto propertyStream = FindStream(reader, "path\\to\\Property");
    std::cout << propertyStream->size << std::endl;
    if (propertyStream == nullptr) {
        std::cerr << "propertyStream is null" << std::endl;
        return 1;
    }
    ExtractFromPropertyStream(reader, propertyStream);
}

I know this code is terrible, but you need to find the errors and fix them.
1) How to ensure that sizeof char== 1?
2) What is the best way to handle errors in this case? For example, in the GetReader function it is not processed that there may be something wrong with fp. Alternatively, throw an exception, handle it in main.
3) How to be with memory leak when it is created unsigned char* bufferin GetReader? I tried unique_ptr, but, as I understand it, because the pointer left the scope, the data was cleared -> access violation
4) What is the best way to process the data?
5) How to improve the code in general?

Answer the question

In order to leave comments, you need to log in

2 answer(s)
T
Ternick, 2020-08-28
@Ternick

1) sizeof(char) always and under any conditions 1 !
2) There is a magic piece SetLastError and GetLastError .
3) There are leaks in your code, as an example:

char* ReadStream(const CFB::CompoundFileReader& reader, const CFB::COMPOUND_FILE_ENTRY* entry) {
    char* buf = new char[entry->size]; // Круто, выделил память и всё
    reader.ReadFile(entry, 0, buf, entry->size); // использовал память
    return buf; // return ?? серьёзно ?? а очистить память ?
}

Read about garbage collectors like cdecl and others.
Create global variables and allocate memory there, and at the end clear, in my opinion it is better to use malloc and free.
I won't be of any further help.

X
xorknown, 2020-08-29
@xorknown

1) The standard guarantees that sizeof(char) == 1
2) In c++, unfortunately, there are not many good ways to handle errors built in, so it's better to use exceptions. They are clear to everyone, you won't have to create CFB::CompoundFileReader in case of an error in GetReader and there won't be a sheet of ifs for each function call. But don't forget to clear memory and close files before throwing an exception. It is better to use RAII .
3) Smart pointers are actually not such bad memory managers, why not use them. From ReadStream, you can simply return unique_ptr, and in the case of a buffer for CFR, if their lifetime should be the same, then why not store them together.

class ReaderWatcher {
public:
    ReaderWatcher(std::shared_ptr<unsigned char[]> b, size_t len)
     : buffer(std::move(b)), reader(b.get(), len) {}

    Reader* operator ->() {
        return &reader;
    }

    Reader get() {
        return reader;
    }

private:
    std::shared_ptr<unsigned char[]> buffer {nullptr};
    Reader reader;
};

Didn't find what you were looking for?

Ask your question

Ask a Question

731 491 924 answers to any question