Bug 3171 - Error in time conversion
Summary: Error in time conversion
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: Miscellaneous (show other bugs)
Version: -current
Hardware: All All
: P5 minor
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks: V_8_4
  Show dependency treegraph
 
Reported: 2020-05-28 13:59 AEST by Ron Frederick
Modified: 2020-10-02 14:55 AEST (History)
1 user (show)

See Also:


Attachments
fix convtime, add unit test (1.83 KB, patch)
2020-05-28 20:49 AEST, Darren Tucker
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ron Frederick 2020-05-28 13:59:21 AEST
While experimenting with the RekeyLimit option, I ran across a small bug in the convtime() function. When I entered a time value of '1m30s', I found that it converted this to 1860 seconds instead of the expected 90 seconds. Entering it as '30s1m' worked fine as a workaround (as would just entering "90"), but clearly the conversion is incorrect.

The bug appears to be that the convtime() function doesn't reset the multiplier back to 1 when it sees the 's'. So, whatever the previous multiplier was (60 in my example) is applied to the number before the 's'. This only works right when a seconds back is specified alone, or first.

Here's the code in question:

                switch (*endp++) {
                case '\0':
                        endp--;
                        break;
                case 's':
                case 'S':
                        break;
                case 'm':
                case 'M':
                        multiplier = MINUTES;
                        break;
                case 'h':
                case 'H':
                        multiplier = HOURS;
                        break;
                case 'd':
                case 'D':
                        multiplier = DAYS;
                        break;
                case 'w':
                case 'W':
                        multiplier = WEEKS;
                        break;
                default:
                        return -1;
                }

One option would be to have a "multiplier = 1" before the "break" for 's'/'S'. There's also a problem with '1m30', though. So, perhaps the better fix is to set the multiplier back to 1 inside the while loop, perhaps right before the switch.
Comment 1 Darren Tucker 2020-05-28 20:49:54 AEST
Created attachment 3400 [details]
fix convtime, add unit test

Thanks for the report.  I agree with your analysis, this should fix it.
Comment 2 Ron Frederick 2020-05-28 23:57:31 AEST
Looks good - thanks for the quick response!
Comment 3 Darren Tucker 2020-05-29 11:24:50 AEST
Thanks, patch has been committed and will be in 8.4.
Comment 4 Darren Tucker 2020-10-02 14:55:03 AEST
Mass close of all bugs fixed in 8.4 release.