Is this right? I mean provided that it still can return error below. I mean wouldn't it be more appropriate to modify destination data only if you return success?

You're right. This logic follows the original asn1_utctime_to_tm function and in that function there is no tmp variable to 'cache' the struct tm (for later usage of adjusting offsets). Since tmp is introduced in this new function, it could be more 'neat' to defer this thing before return success.

Do you mind either adding parenthesis around first expression, or splitting these two declarations to separate lines. Note that second one is preferred according to style guide. And while you are at it throw in empty line after declaration.

OK, I prefer to split it into two lines.

One can ask same question whether or not it's appropriate to modify *tm if asn1_time_to_tm() returns error. Why not simply let asn1_time_to_tm() decide? There is certain ambiguity in documentation, it says "Only the B<tm_sec>, B<tm_min>, B<tm_hour>, B<tm_mday>, B<tm_mon> and B<tm_year> fields are updated." It sounds like that if you pass a tm if it won't touch fields other than listed. This doesn't seem to be true. I mean it wasn't true to start with... Hmmm...

But if documentation's intention was to say that it returns only these fields [zeroing all others], then one can probably argue that one should do just that. That is call asn1_time_to_tm(&tmp,...) and copy listed fields...

Ah! Disregard comment implied suggestion to harmonize with documentation by copying listed fields. Because those are only fields set by underlying subroutine. Sorry about confusion. So there is only one question, about letting asn1_time_to_tm() to decide if *tm is to be modified. There is memset in asn1_time_to_tm(), so it will be equivalent...

Though one can wonder if documentation should be refined, e.g. by stating that only listed fields are set to meaningful values.

So the problem is how should the ASN1_TIME_to_tm be designed. There might be 2 choices:

1. set the listed (in doc) fields of tm to proper values, and zero all others.
2. set the listed fields of tm to proper values, and leave other fields as-is.

For both choices, it's only applied if the 'TIME' s is parsed successfully, otherwise leave tm untouched.

I prefer the first choice, and I guess this is what ASN1_TIME_to_tm intends to be. If a user wants to just fill the listed fields, he/she could call something like ASN1_TIME_to_tm(s, &tmp) and copy the fields of tmp to other tm/places by his/her own. That's not the scope covered by this API.

So the memset stuff in ASN1_TIME_to_tm should be removed and documentation of ASN1_TIME_to_tm should be modified to match this behavior either.

And also, in this scenario, if a NULL tm is passed to ASN1_TIME_to_tm and finally is passed to asn1_time_to_tm, the ASN1_TIME_to_tm will perform in a 'format-checking-only' mode, which is similar to what ASN1_TIME_set_string does. This feature should also be documented, of course.

Makes sense to what you suggested?

If you examine what are the remaining fields of the struct tm, i.e. those not listed, you'll see that they are tm_wday, tm_yday and tm_isdst. Confusion arose from the assumption that that there are more fields, or rather such that would be considered as worth preserving. Once again, wording "only fields x, y, z, are updated" kind of implies that caller is entitled to expect that other fields are preserved and worth preserving. But after closer look it's obvious that it's essentially unnatural to expect this. It's only natural that non-listed fields are zeroed. And that's what happens, and even without referred memset, asn1_time_to_tm does it in either case.

To summarize, bottom line is that it's sufficient to omit memset, and in additoin to that I'd argue in favour of rewording documentation to avoid the misconception.

This doesn't belong here. As already discussed *tm modification is done only at the very end and if there were no errors.

Ahh, this is definitely a bug, tmp should be updated instead of tm. And it seems current test cases don't cover this...

Good catch!

Ahh, this is definitely a bug, tmp should be updated instead of tm. And it seems current test cases don't cover this...

Hmm, tmp is also not needed to be set to 0, since we already have memseted all tmp to 0 previously. So this piece of code will not cause problems, but it's indeed unnecessary to exist. For test cases, I have checked it again, the no 'SS' (seconds) situation is already covered, so that's great : )

[edited: quote previous comments to make the statements more clear]

Can one eliminate this condition? I mean would it be inappropriate to make the adjustment if tm is NULL?

And if it's inappropriate, tmp != NULL is considered to be more appropriate.

This might not be a problem. The idea is, tmp is always there and is filled with data no matter if tm is NULL or not. So if tm is NULL, there is no need to update tm by tmp and tmp will silently be ignored. So if tm is NULL, no need to adjust tmp neither.

But there is a coding style problem. if (tm) to if (tm != NULL) ...

Also, I guess you would ask that, why other fields of tmp are not updated based on the presence of tm in the above 'digital-parsing' loop. That is because the adjustment operation performed in OPENSSL_gmtime_adj is more heavy than those variables assignments, so I think put a tm != NULL check here only for adjustment makes sense.

Is it necessary to modify the mdays array here? Change the += to + and declare mdays as static const above.

Filling tm_wday and tm_yday is straightforward. Is there any reason this hasn't been done?

This isn't a request to do it.

I don't know why these 2 fields are not filled neither. Is it possible that not all struct tm has those fields on any platform?

Besides, is it really straightforward to calculate tm_wday based on the given year-month-day info?

The structure in question is actually specified in C language standard, so that probability of the 2 fields in question not being available on some platform is virtually zero. Or at least most portability problems of this kind are attributed rather to POSIX than to C language.

The weekday is straightforward. Zeller's congruence is the magic to search for.
I'm happy to add this later as another PR, I've written this code before and would just have to adjust it to match.

And add test cases.

s/th/to/