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:

Quote:

---

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.

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).

Quote:

---

Originally Posted by Salem

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.

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.

Quote:

---

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

Quote:

---

Originally Posted by jimblumberg

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.

Quote:

---

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