Results 1 to 3 of 3

Thread: An Issue about Zip Archive Retrieval Caused by Unwanted Memory Reuse.

  1. #1

    Post An Issue about Zip Archive Retrieval Caused by Unwanted Memory Reuse.

    Dear All,


    I noticed that when I tried to retrieval all directories in a zipped archive by "open_directory()" recursively on a virtual file system, I always got incorrect results. That is, the results are incomplete.

    For example. if I have a zipped archive with the following file structure:
    /----
    |- Letters
    | |- A
    | |- B
    | |- C
    |- Numbers
    |- 0
    |- 1
    |- 2
    and I use the following code to recursively travel through the zipped archive illustrated above.

    Code:
    #include "stdafx.h"
    
    CL_VirtualFileSystem* vfs = NULL;
    void enum_dirs(CL_String path);
    
    int _tmain(int argc, _TCHAR* argv[])
    {
    	CL_SetupCore setup_core;
    
    	vfs = new CL_VirtualFileSystem("Issue01.zip", true);
    
    	enum_dirs("");
    
    	delete vfs; vfs = NULL;
    }
    
    void enum_dirs(CL_String path)
    {
    	CL_VirtualDirectoryListing vdl = vfs->open_directory(path).get_directory_listing();
    
    	while (vdl.next())
    	{
    		if (vdl.is_directory())
    		{
    			CL_String dir_name = vdl.get_filename();
    			if (0 != dir_name.compare(".") && 0 != dir_name.compare(".."))
    			{
    				CL_String abs_name = path + "/" + vdl.get_filename();
    				CL_Console::write_line(abs_name);
    				enum_dirs(abs_name);
    			}
    		}
    	}
    }
    Both the code (Visual Studio 2010 project) and the zipped archive file are attached with this post.

    Issue01 Demo Code.zipIssue01.zip


    The outcome of the test illustrated above is (unfortunately incorrect):

    /Letters
    /Letters/A



    That is: some directories in this zipped archive are missing!


    By tracking down the code, I think that this error is caused by a problem in the "zip_archive.cpp" file. To be precise, the "added_directories" local variable declared at line 98 in the "get_file_list" member function in the "CL_ZipArchive" class.

    As in the code, this local variable is declared as: "std::vector<CL_StringRef> added_directories;" That is: the element of this vector is not the actual string, but is a reference to the actual string. I guess it was designed in this way to improve performance and/or reduce memory usage. However, at line 131 of the same method, this variable ("added_directories") is populated with the value of another local string variable: "directory_name".

    There are issues need to be mentioned:
    1) what being kept in the "added_directories" is the reference to a string and is not a full copy of the string.
    2) the address being referenced, the actual string ("directory" in this case) is a local variable declared inside a for loop (line 100 is the for loop and line 116 is where the "directory_name" being declared).

    The consequence of the combination of the two factors mentioned above is that the actual content of the element in the "added_directories" are always changing as long as the "directory_name" being assigned to a new value. That is because Windows OS is likely to reuse the same address for each iteration of the "directory_name" being declared inside a loop.

    In my example. The first directory is added into the "added_directories" element 0 is "Letters". Then, for directories "A", "B" and "C", they are fine because these directories are under "Letters". When the search process goes to "Numbers", both the "directory_name" and the first element of the "added_directories" changed at the same time at line 116 when the new "directory_name" variable is declared as the same memory address is reused. This is the cause of the error in my opinion.


    Suggested change:

    I recommend that the Line 98 in the source code file "zip_archive.cpp" shall be changed from "std::vector<CL_StringRef> added_directories;" to "std::vector<CL_String> added_directories;". That is: use "CL_String" instead of "CL_StringRef".

    I also recommend that similar usages of "CL_StringRef" in other parts of the code (if this is any) shall be re-examined.



    Best regards,

    Chris

  2. #2
    ClanLib Developer
    Join Date
    May 2007
    Posts
    1,824

    Default

    Many thanks for that detailed explanation.

    Patch applied to ClanLib 2.3 SVN and ClanLib 2.4 SVN

    Ideally, CL_StringRef would be removed, and CL_String would use "copy-on-write". But I am unsure of the overhead.
    Code:
    std::string x("Hello");
    std::string y = x;  // x and y use the same buffer
    y += ", World!";    // now y uses a different buffer
                        // x still uses the same old buffer
    (From http://en.wikipedia.org/wiki/Copy_on_write )

  3. #3

    Smile

    Quote Originally Posted by rombust View Post
    Many thanks for that detailed explanation.

    Patch applied to ClanLib 2.3 SVN and ClanLib 2.4 SVN

    Ideally, CL_StringRef would be removed, and CL_String would use "copy-on-write". But I am unsure of the overhead.
    Code:
    std::string x("Hello");
    std::string y = x;  // x and y use the same buffer
    y += ", World!";    // now y uses a different buffer
                        // x still uses the same old buffer
    (From http://en.wikipedia.org/wiki/Copy_on_write )



    Hi rombust,



    You are very welcome!

    ClanLib is the best ever game engine in my opinion. It is my pleasure to do something for this fabulous library.


    Best regards,

    Chris

Similar Threads

  1. Reading from memory in Clan 1.0
    By speeder in forum Official ClanLib SDK Forums
    Replies: 1
    Last Post: 07-12-2010, 12:28 PM
  2. Memory lost
    By keph in forum Official ClanLib SDK Forums
    Replies: 2
    Last Post: 09-22-2009, 08:22 AM
  3. Video Memory
    By EdK in forum Official ClanLib SDK Forums
    Replies: 2
    Last Post: 06-14-2008, 08:39 PM
  4. Memory Leaks
    By in forum Funeral Quest
    Replies: 9
    Last Post: 03-06-2007, 11:00 AM
  5. Password Retrieval
    By in forum Funeral Quest
    Replies: 2
    Last Post: 12-21-2002, 08:10 PM

Bookmarks

Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •