Hi Klaus, > * the public and private object loading functions are using > PATH_MAX for the file names but reading a harcoded "50" bytes for the > filename in the index file?
True, although this happens elsewhere in openCryptoki too, lots of magic numbers. > * We're not checking the return value for fread() when dealing with > the size and "is_private" for the objects. Corruption could, in theory, > happen in those two first fields as well, right? Corruption yes, but we're reading fixed sizes in those cases, so the overflow case doesn't happen there. > * Is it a good idea mixing up CK_ULONG_32 size from the object loading > functions with a simple "int" from the *_withSize() functions? Should > we be using arch-independent types and perhaps defining max object > sizes? Not sure what you mean by mixing up? I chose the int type so that -1 could be used as a flag for "don't check", Axel's original patches did use and unsigned but that caused compile warnings. > * Perhaps we could define a new "Token object corrupted" standard > message for st_err_log() and use that instead? In fact I hate how we Yeah, I fully expected those to get replaced when I do the updates for logging. > have st_err_log(), LOG(), and several other things for the same > purpose. I saw we are including <syslog.h> for the sole purpose of > using LOG_ERR also. I won't mind since I know we have been planning > ontemplate_unflatten_withSize refactoring this area anyway. Maybe we > could take the same approach as we did in the testcases? i.e., > #define LOG_ERR(msgcode_enum, fmt_string, ...) > same for LOG_WARN(), LOG_DEBUG(), perhaps even a special LOG_TRACE()... Yep, this is a good idea. > * I don't think we use CamelCase anywhere else in the code. True > * I saw we are checking for "NULL" pValues in attributes, but where are > NULL values assigned if they are found to be corrupted to begin with? > Seems like template_unflatten_withSize() is only assigning pValue with > NULL if ulValueLen is zero, a check that was being made anyway. I don't have a testcase for this one, so you may be right that the checks are rundundant... Kent > -Klaus > > > > -- > Klaus Heinrich Kiwi | [email protected] | http://blog.klauskiwi.com > Open Source Security blog : http://www.ratliff.net/blog > IBM Linux Technology Center : http://www.ibm.com/linux/ltc ------------------------------------------------------------------------------ This SF.net Dev2Dev email is sponsored by: Show off your parallel programming skills. Enter the Intel(R) Threading Challenge 2010. http://p.sf.net/sfu/intel-thread-sfd _______________________________________________ Opencryptoki-tech mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opencryptoki-tech
