Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sun, 17 Mar 2013 23:14:53 +0100
From: magnum <john.magnum@...hmail.com>
To: john-dev@...ts.openwall.com
Subject: Re: All *2john programs should use basename of filename when put as “login” field

On 17 Mar, 2013, at 22:54 , Lukas Odzioba <lukas.odzioba@...il.com> wrote:
> 2013/3/17 magnum <john.magnum@...hmail.com>:
>> As far as I know there are two problems:
>> 1. Some versions of basename() modify the original string, some do not. This is only a problem if we want to keep the original full name as well.
>> 2. Some versions of basename() may return NULL on error. This problem might be more or less academic for this use.
>> 
>> The current gpg2john code handles both cases. A generic version in misc.c or maybe in path.c is a good idea. Supporting an arbitrary list of extensions to strip would be powerful of course.
> 
> misc.s seems to be better place.
> What do you think about this version:
...
> char *get_basename(const char *path, const char *extensions[], int count)
> {
>        assert(path);
>        assert(*path);

I think we should avoid asserts other than for debugging. We could opt to return a valid null string:

	if (!path || !*path)
		return "";


>        static char retmem[64];

I think I'd use at least 256 here.


>        char *pathdup = NULL;
>        char *base = NULL;
>        int retsize,i,extlen,baselen = 0;
>        pathdup = strdup(path);
>        if(!pathdup)
>                goto error;
>        base = basename(pathdup);
>        if(!base || !*base)
>                goto error;
>        baselen = strlen(base);
> 
>        while(count--) {
>                extlen=strlen(extensions[count]);
>                if ( baselen > extlen &&
> !strcmp(&base[baselen-extlen],extensions[count]) ) {
>                        base[baselen-extlen]=0;
>                        break;
>                }
>        }
> 
>        retsize = strlen(base) + 1;
>        memcpy(retmem, base, retsize<64?retsize:64);

The above does not guarantee a null termination. Replace the two lines with strnzcpy(retmem, base, sizeof(retmem)), conveniently also located in misc.c.


>        free(pathdup);
>        return retmem;
> 
>        error:
>                if(pathdup)
>                        free(pathdup);
>                return path;
> }



The rest looks fine to me, but I'm not exactly a skilled code reviewer :)

magnum

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.