patterncppMinor
Parsing arguments for cd command
Viewed 0 times
commandargumentsparsingfor
Problem
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 ConvertCDArgToPath( const unicode_t* p )
{
std::vector Out;
std::vector 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 == '
- Is this code secure?
- What i
)
{
// skip `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 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;
}- Is this code secure?
- What i
Solution
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:
How to deal with the duplicate code around
I guess, the simplest way is to move the duplicated code directly into
Naming
It is very unusual to have variable names capitalized.
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() callsI 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.
Context
StackExchange Code Review Q#64150, answer score: 2
Revisions (0)
No revisions yet.