Thread: Setting Icon Locations of Desktop Shortcuts: Segmentation Fault

  1. #1
    Registered User
    Join Date
    Dec 2014
    Posts
    3

    Setting Icon Locations of Desktop Shortcuts: Segmentation Fault

    I've been attempting to write code using COM on my Windows 7 computer with MingW and C++11. For some reason, I've been receiving SIGSEGV through my debugger in the DesktopShortcut::SetIcon function after I try to set it back to the old icon path I got from DesktopShortcut::GetIcon. I've tested COM::Assert and it has caught all unsuccessful COM calls, so I believe it to be an issue in how I handle the std::wstring class (particularly in the DesktopShortcut/Files classes). I'm new to C++ so I'm not always aware of object lifetimes and scopes. Here's the relevant code:

    wmain:

    Code:
    int wmain( int argc, wchar_t * argv[] )
    {
        const std::wstring ICON_FOLDER_PATH = std::wstring( Files::GetBasePath( argv[0], wcslen( argv[0] ) ) ) + L"Icons";
        Random random;
    
    
        std::vector<std::wstring> paths;
    
    
        auto lambda = [&]( std::wstring& path, const wchar_t * fileName ) -> bool
        {
            paths.emplace_back( path + fileName );
            return true;
        };
    
    
        try
        {
            Files::Enumerate( lambda, ICON_FOLDER_PATH );
            Desktop desktop( true );
    
    
    
    
            for ( auto& shortcut : desktop.GetShortcuts( ) )
                shortcut.SetIcon( random.GetElement<std::wstring>( paths ).c_str( ) );
        }
        catch ( const wchar_t * description )
        {
            std::wcerr << description << L"\n";
        }
        catch ( std::exception &e )
        {
            std::wcerr << e.what( ) << L"\n";
        }
    
    
        return 0;
    }
    Files.cpp
    Code:
    bool Files::EndsWith( std::wstring& s, std::wstring& sub )
    {
        const size_t sLength = s.length( );
        const size_t subLength = sub.length( );
    
    
        if ( sLength >= subLength )
            return s.compare( sLength - subLength, subLength, sub ) == 0;
    
    
        return false;
    }
    
    
    void Files::Enumerate( std::function<bool(std::wstring&, const wchar_t * )> callback, std::wstring path, bool recurse )
    {
        bool success = true;
        WIN32_FIND_DATA data = { 0 };
    
    
        if ( path[path.length( ) - 1] != L'\\' ) path += L"\\";
        HANDLE directory = FindFirstFile( ( path + L"*" ).c_str( ), &data );
    
    
        if ( ( success = ( directory != INVALID_HANDLE_VALUE ) ) )
        {
            do
            {
                if ( wcscmp( data.cFileName, L"." ) && wcscmp( data.cFileName, L".." ) )
                {
                    if ( data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY )
                    {
                        if ( recurse )
                            Files::Enumerate( callback,  path + std::wstring( data.cFileName ), recurse );
                    }
                    else
                        if ( !callback( path, data.cFileName ) )
                            break;
                }
            } while ( FindNextFile( directory, &data ) );
    
    
            FindClose( directory );
        }
    
    
        if ( !success )
        {
            Error::Windows( );
            throw L"Files::Enumerate(): FindFirstFile";
        }
    }
    
    
    void Files::Enumerate( std::function<bool(std::wstring&, const wchar_t * )> callback, std::wstring path, std::wstring extension, bool recurse )
    {
        auto lambda = [&]( std::wstring& path, const wchar_t * fileName ) -> bool
        {
            std::wstring name( fileName );
            if ( Files::EndsWith( name, extension ) )
                return callback( path, fileName );
    
    
            return true;
        };
    
    
        Files::Enumerate( lambda, path, recurse );
    }
    
    
    const wchar_t * Files::GetBasePath( wchar_t * buffer, size_t length )
    {
        for ( size_t i = length - 1; buffer[i] != L'\\' && i >= 0; --i )
            buffer[i] = '\0';
    
    
        return buffer;
    }
    Desktop.cpp:

    Code:
    Desktop::Desktop( bool revert ) : EXTENSION( L".lnk" ), revert( revert )
    {
        Com.Assert( SHGetKnownFolderPath( FOLDERID_Desktop, 0, nullptr, &path ), L"SHGetKnownFolderPath( FOLDERID_DESKTOP, ... )" );
    }
    
    
    const wchar_t * Desktop::GetPath( )
    {
        return path;
    }
    
    
    std::vector<DesktopShortcut>& Desktop::GetShortcuts( )
    {
        if ( !shortcuts.empty( ) )
            shortcuts.clear( );
    
    
        Files::Enumerate
        (
            [&]( std::wstring& filePath, const wchar_t * fileName ) -> bool
            {
                shortcuts.emplace_back( ( filePath + fileName ).c_str( ), revert );
                return true;
            },
            path,
            EXTENSION,
            false
        );
    
    
        return shortcuts;
    }
    
    
    Desktop::~Desktop( )
    {
        CoTaskMemFree( path );
    }
    DesktopShortcut.cpp

    Code:
    DesktopShortcut::DesktopShortcut( const wchar_t * path, bool revert ) : revert( revert )
    {
        ShellLink = ( IShellLinkW * )Com.GetObject( CLSID_ShellLink, IID_IShellLink );
        PersistFile = ( IPersistFile * )Com.GetInterface( ShellLink, IID_IPersistFile );
    
    
        Com.Assert( PersistFile->Load( path, 0 ), path );
        Com.Assert( ShellLink->Resolve( nullptr, SLR_NO_UI ),  L"ShellLink->Resolve" );
    
    
        std::wcout << path << L"\n";
        if ( revert );
            this->GetIcon( &oldIconPath, &oldIconIndex );
    }
    
    
    void DesktopShortcut::SetIcon( const wchar_t * path, int index )
    {
        Com.Assert( ShellLink->SetIconLocation( path, index ), L"ShellLink->SetIconLocation" );
        this->Save( );
    }
    
    
    void DesktopShortcut::GetIcon( std::wstring ** path, int * index )
    {
         *path = new std::wstring( MAX_PATH, '\0' );
         std::wcout << *path << L"\n";
         Com.Assert( ShellLink->GetIconLocation( &( **path )[0], MAX_PATH, index ), L"ShellLink->GetIconLocation" );
         std::wcout << ( **path ) << L"\n";
         ( *path )->resize( wcslen( ( *path )->c_str( ) ) );
         std::wcout << ( **path ) << L"\n";
    }
    
    
    void DesktopShortcut::GetIcon( std::wstring ** path )
    {
        int foo = 0;
        this->GetIcon( path, &foo );
    }
    
    
    void DesktopShortcut::GetIcon( int& path )
    {
        std::wstring ** foo = nullptr;
        this->GetIcon( foo, &path );
        delete foo;
    }
    
    
    void DesktopShortcut::Save( )
    {
        Com.Assert( PersistFile->Save( nullptr, false ), L"PersistFile->Save" );
        PersistFile->SaveCompleted( nullptr );
    }
    
    
    DesktopShortcut::~DesktopShortcut( )
    {
        if ( revert )
        {
            this->SetIcon( ( *oldIconPath ).c_str( ), oldIconIndex );
            delete oldIconPath;
        }
    }
    I get output such as the following before it crashes:

    C:\Users\Chris\Desktop\CodeBlocks.lnk
    0x3a84a0










    C:\Users\Chris\Desktop\Eclipse Java Mars.lnk
    0x3a7878










    C:\Users\Chris\Desktop\Excel 2013.lnk
    0x3a7ef8
    Many thanks, OldManJenkins.

  2. #2
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    39,666
    First of all, everything calling the Win32 API directly with strings should be passed things declared as _TCHAR
    https://msdn.microsoft.com/en-us/library/c426s321.aspx


    > Com.Assert( ShellLink->GetIconLocation( &( **path )[0], MAX_PATH, index ), L"ShellLink->GetIconLocation" );
    So something like
    Code:
    _TCHAR temp[MAX_PATH];
    Com.Assert( ShellLink->GetIconLocation( temp, MAX_PATH, index ), _TEXT("ShellLink->GetIconLocation") );
    You can use std::wstring in your own code, but you need to make sure that the API calls themselves are passed the correct types, and actually pointing to valid memory.

    Good
    Code:
    _TCHAR temp[MAX_PATH];
    Com.Assert( ShellLink->GetIconLocation( temp, MAX_PATH, index ), _TEXT("ShellLink->GetIconLocation") );
    Bad
    Code:
    _TCHAR *temp;
    Com.Assert( ShellLink->GetIconLocation( temp, MAX_PATH, index ), _TEXT("ShellLink->GetIconLocation") );
    The compiler is happy - the types of things resolve correctly.
    The run-time however is broken since the pointer isn't actually referencing any valid memory (the result being the segmentation fault).
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.

  3. #3
    Registered User
    Join Date
    Dec 2014
    Posts
    3
    Quote Originally Posted by Salem View Post
    First of all, everything calling the Win32 API directly with strings should be passed things declared as _TCHAR
    https://msdn.microsoft.com/en-us/library/c426s321.aspx


    > Com.Assert( ShellLink->GetIconLocation( &( **path )[0], MAX_PATH, index ), L"ShellLink->GetIconLocation" );
    So something like
    Code:
    _TCHAR temp[MAX_PATH];
    Com.Assert( ShellLink->GetIconLocation( temp, MAX_PATH, index ), _TEXT("ShellLink->GetIconLocation") );
    You can use std::wstring in your own code, but you need to make sure that the API calls themselves are passed the correct types, and actually pointing to valid memory.

    Good
    Code:
    _TCHAR temp[MAX_PATH];
    Com.Assert( ShellLink->GetIconLocation( temp, MAX_PATH, index ), _TEXT("ShellLink->GetIconLocation") );
    Bad
    Code:
    _TCHAR *temp;
    Com.Assert( ShellLink->GetIconLocation( temp, MAX_PATH, index ), _TEXT("ShellLink->GetIconLocation") );
    The compiler is happy - the types of things resolve correctly.
    The run-time however is broken since the pointer isn't actually referencing any valid memory (the result being the segmentation fault).
    I define _UNICODE and UNICODE in a header file so that all the functions resolve to their Unicode versions. I thought _TCHAR wraps for ANSI or Unicode type characters depending upon what preprocessor directives are declared. Also, why doesn't this code:
    Code:
    *path = new std::wstring( MAX_PATH, '\0' );
    allocate "valid memory"? I thought that it would allocate a Unicode string filled with MAX_PATH null-terminating characters in the std::wstring object.
    Last edited by OldManJenkins; 03-09-2016 at 12:57 PM.

  4. #4
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    39,666
    TCHAR wraps everything so you don't have to care about whether it's ASCII or UNICODE.

    > Also, why doesn't this code:
    > *path = new std::wstring( MAX_PATH, '\0' );
    > allocate "valid memory"?
    Whilst it does indeed allocate memory, it doesn't immediately infer that path itself is pointing to MAX_PATH consecutive elements of wide characters.

    Standard C++ containers are typically fixed sized headers pointing to variable length data, eg.
    Code:
    class std::wstring {
      size_t allocated;
      wchar_t *buffer;
    };
    If you print sizeof(std::wstring) or sizeof(*path), you're always going to get the same small number as a result, regardless of how big you make the actual string.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.

  5. #5
    Registered User
    Join Date
    May 2010
    Posts
    4,632
    Also, why doesn't this code:
    The better question is why are you using a pointer to a std::wstring? Why not just use a normal std::wstring instead?

    Code:
    #include <iostream>
    #include <string>
    
    using namespace std;
    
    int main()
    {
        const size_t MAX_PATH = 2056;
    
        std::wstring path( MAX_PATH, '\0' );
    
        return 0;
    }
    After all a std::wstring is dynamic all by it's self so you shouldn't need the dynamic memory mess.


    Jim

  6. #6
    Registered User
    Join Date
    Dec 2014
    Posts
    3
    Quote Originally Posted by jimblumberg View Post
    The better question is why are you using a pointer to a std::wstring? Why not just use a normal std::wstring instead?

    Code:
    #include <iostream>
    #include <string>
    
    using namespace std;
    
    int main()
    {
        const size_t MAX_PATH = 2056;
    
        std::wstring path( MAX_PATH, '\0' );
    
        return 0;
    }
    After all a std::wstring is dynamic all by it's self so you shouldn't need the dynamic memory mess.


    Jim
    I don't want the object to destruct itself at the end of the function.

  7. #7
    Registered User
    Join Date
    May 2010
    Posts
    4,632
    I don't want the object to destruct itself at the end of the function.
    What? If you return the string from the function it won't be destructed when the function ends.

    Jim

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. In GDB no segmentation fault but while running segmentation fault
    By Tamim Ad Dari in forum C++ Programming
    Replies: 2
    Last Post: 12-10-2013, 11:16 AM
  2. Setting desktop background color
    By Brian in forum Windows Programming
    Replies: 10
    Last Post: 08-21-2009, 11:34 AM
  3. displaying an icon on desktop
    By parviin57 in forum Windows Programming
    Replies: 3
    Last Post: 12-23-2006, 12:23 PM
  4. Setting a Dialogbox Icon
    By Devil Panther in forum Windows Programming
    Replies: 1
    Last Post: 12-08-2004, 02:17 PM
  5. Creating a desktop icon from code
    By knutso in forum Windows Programming
    Replies: 5
    Last Post: 12-28-2002, 09:56 AM

Tags for this Thread