3
\$\begingroup\$

This file manager has a code snippet that handles an argument of the "cd" console command. It converts the argument to a valid path, expands ~ and environment variables into the corresponding values:

std::vector<unicode_t> ConvertCDArgToPath( const unicode_t* p )
{
    std::vector<unicode_t> Out;

    std::vector<unicode_t> Temp;

    while ( p && *p )
    {
        unicode_t Ch = 0;

        if ( *p == '~' )
        {
            if ( LookAhead( p, &Ch ) )
            {
                if ( ( IsPathSeparator(Ch) || Ch == 0 ) && ApplyEnvVariable( "HOME", &Temp ) )
                {
                    // replace ~ with the HOME path
                    Out.insert( Out.end(), Temp.begin(), Temp.end() );
                    PopLastNull( &Out );
                }
            }
        }
        else if ( *p == '$' )
        {
            // skip `$`
            std::vector<char> EnvVarName = unicode_to_utf8( p+1 );
            for ( auto i = EnvVarName.begin(); i != EnvVarName.end(); i++ )
            {
                if ( IsPathSeparator( *i ) )
                {
                    *i = 0;
                    break;
                }
            }

            if ( ApplyEnvVariable( EnvVarName.data( ), &Temp ) )
            {
                // replace the var name with its value
                Out.insert( Out.end(), Temp.begin(), Temp.end() );
                PopLastNull( &Out );
                // skip var name
                p += strlen( EnvVarName.data() );
            }
        }
        else if ( IsPathSeparator(*p) )
        {
            if ( !LastCharEquals( Out, '/' ) && !LastCharEquals( Out, '\\' ) ) Out.push_back( DIR_SPLITTER );
        }
        else
        {
            Out.push_back( *p );
        }
        p++;
    }

    Out.push_back( 0 );

    return Out;
}

bool LookAhead( const unicode_t* p, unicode_t* OutNextChar )
{
    if ( !p ) return false;
    if ( !*p ) return false;
    if ( OutNextChar ) *OutNextChar = *(p+1);
    return true;
}
  1. Is this code secure?
  2. What is missing?
  3. How can I deal with the duplicate code around the ApplyEnvVariable() calls?
\$\endgroup\$
  • \$\begingroup\$ Isn't ~ expanded by the shell automatically when used as parameter? \$\endgroup\$ – edmz Sep 29 '14 at 13:58
  • \$\begingroup\$ This code is not for the shell. The file manager handles the cd command internally to switch to the desired folder. I.e. cd ~/Projects/$PROJNAME/data should work. \$\endgroup\$ – Sergey K. Sep 29 '14 at 13:59
  • \$\begingroup\$ @SergeyK. most shells already do all this for you why are you trying to do it inside the command? \$\endgroup\$ – Martin York Sep 29 '14 at 17:57
  • \$\begingroup\$ This is the wrong place to ask this question: Is this code secure? \$\endgroup\$ – Martin York Sep 29 '14 at 17:57
  • \$\begingroup\$ @LokiAstari: will it allow me to read back the result of the command? The path to the resulting folder. \$\endgroup\$ – Sergey K. Sep 29 '14 at 21:11
2
\$\begingroup\$

What is missing

A compatibility with existing shells. There are certain expectations on how the expansion works; your code expands in an incompatible way; expect end-user complains:

  • Tilde is expanded only in the beginning of the word. You expand it anywhere.
  • Tilde may be followed by a login name, then the whole prefix is expanded to a home directory of that user. You do not expand the tilde-prefix.
  • There is no provision to escape tilde or $ from expanding.
  • There is no way to limit variable name before it hits the path separator.

How to deal with the duplicate code around ApplyEnvVariable() calls

I guess, the simplest way is to move the duplicated code directly into ApplyEnvVariable(). That would actually justify the name of the method: as of now it doesn't apply the variable but merely expands it. Of course you'd need to pass Out.end() instead of Temp there.

Naming

It is very unusual to have variable names capitalized.

\$\endgroup\$
  • \$\begingroup\$ Nice list of compatibility issues! \$\endgroup\$ – Sergey K. Sep 29 '14 at 21:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.