Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix shared memory read mode potential issue #2032

Closed
wants to merge 2 commits into from

Conversation

WeiChungChang
Copy link

@WeiChungChang WeiChungChang commented Dec 3, 2017

Current when using SharedMemory, there are 2 issues for read mode.

  1. There are two kinds of constructors, :
1. SharedMemory::SharedMemory(const std::string& name, std::size_t size, AccessMode mode, const void* addrHint, bool server)
2. SharedMemory::SharedMemory(const Poco::File& file, AccessMode mode, const void* addrHint)

They both accept parameters AccessMode mode.
However, with 1st constructor, SharedMemory::AM_READ should implicitly be called with sever = false.
Notice that parameter sever has a default value = true
It often confuses the user who are NOT so familiar with Poco::ShareMemory since if SharedMemory::AM_READ is used with default value of sever = true, will make system core dump.

Furthermore, for the example provided in SharedMemoryTest.cpp, only the function calls without setting sever (with default value = true) are provided.

1. SharedMemory mem("hi", 4096, SharedMemory::AM_WRITE);
2. SharedMemory mem(f, SharedMemory::AM_READ);

Hence make a call to
SharedMemory mem("hi", 4096, SharedMemory::AM_READ);
will lead to system exception:

terminate called after throwing an instance of 'Poco::SystemException'
what(): System exception
Aborted (core dumped)

It needs the user to figure out they should call
SharedMemory mem("hi", 4096, SharedMemory::AM_READ, 0, false);
by checking source code themselves.

So the 1st proposed fix is to decide sever by AccessMode since read mode should not be master

  1. The 2nd fix tries to prevent a size mismatch problem.

Please notice that the logic below is legal now.

int main(int argc, char **argv) {
    int number = atoi (argv[1]);
    size_t pageSize = 4096;
    if(number == 0) { // main process.
        size_t w_sz = pageSize;
        SharedPtr<SharedMemory> p1 = new SharedMemory("test", w_sz,  SharedMemory::AM_WRITE);
        char* ptr = p1->begin();
        int length = p1->end() - p1->begin();
        for (int i = 0; i < length; i++) {
            ptr[i] = '0' + (i % 10);
        }
        std::system("./test 1");
    } else if (number == 1) { // child process
        size_t r_sz = pageSize * 2;
        SharedPtr<SharedMemory> p2 = new SharedMemory("test", r_sz, SharedMemory::AM_READ, 0, false);
        char* ptr = p2->begin();
        int length = p2->end() - p2->begin();
        for (int i = 0; i < length; i++) {
           printf("%dth %c\n", i, ptr[i]);
        }
    }
    return 1;
}

This example will finally lead to core dump when child process tries to read out of a page (4096 here) boundary.
In fact, currently the size if not checked so you can assign the reader a size which is larger than the writer (master). If it does NOT reach the limit of the boundary of a page, the part of over-assigned will be filled with 0 by OS for page alignment. However, once it exceeds the page boundary, we get core-dump. It is potentially dangerous.

Hence a fix is provided to compare the size and the file size; if the requested read size is larger than the actual size we have, we trim it to actual size for updating the internal info of the mapped shared memory.
The users may still need to check the real mapped size, ex: by looking size_t allocated = p1->end() - p1->begin(). But at least, the size will be an legal value instead of a casual one which could lead to system crash.

@@ -32,7 +32,7 @@ SharedMemoryImpl::SharedMemoryImpl(const std::string& name, std::size_t size, Sh
_access(mode),
_name("/"),
_fileMapped(false),
_server(server)
_server((mode == SharedMemory::AM_WRITE))
Copy link
Member

@aleks-f aleks-f Dec 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if bool server is not needed/used, it should be removed from the signature and documentation updated

@@ -58,6 +58,17 @@ SharedMemoryImpl::SharedMemoryImpl(const std::string& name, std::size_t size, Sh
_fd = -1;
::shm_unlink(_name.c_str());
throw SystemException("Cannot resize shared memory object", _name);
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants