Duration Conversion

[bcpkh]

[C application running on TRU64 Unix]

Hello All

I have a simple task that is driving me crazy. A string representing a
duration in the following format is passed to my application, a
function is dedicated to convert this duration to seconds;

[H]H:MM:SS

e.g. 0:00:00 or 00:12:45

The original function used atoi as part of the conversion process and
it produced some scary conversions 0:00:10 converted to 956 seconds
etc.

The function was rewritten to make use of strtol because of its better
error handling functionality but it now core dumps! Do someone have a
bullet proof idea to do this conversion assuming that anything might
be passed into this function;

e.g. A:00:00 or :0:45 etc

Reproducted below is the offending function the lines where it
currently dumps is marked with a @.

Any help would be much appreciated.

Thank you,

B

void secondDurations()
{
long hr, min, sec, sec_duration = 0;
char tm[10];
int ch = ':';
char *end_ptr = NULL;
char duration[41];

char *pfirst;
char *plast;

// 00:00:00
// 0:00:00
// 01234567


strcpy(duration, "00:00:34");

//String should at leat be 7 characters long
if (strlen(duration) >= 7)
{
pfirst = strchr(duration, ch);
plast = strrchr(duration, ch);

//Test duration format 00:00:00
if((pfirst != NULL || plast != NULL) && (pfirst != plast))
{
errno = 0;

//Get hour portion
strcpy(tm, strtok(duration,":"));

//Convert
hr = strtol(tm, &end_ptr, 10);

//Test
if (hr > INT_MAX || hr < INT_MIN)
{
//Reject;
return;
}
else if (end_ptr == tm || *end_ptr != '\0')
{
//Reject
return;
}
else
{
errno = 0;

@ strcpy(tm, strtok(NULL,":"));

min = strtol(tm, &end_ptr, 10);

if (min > INT_MAX || min < INT_MIN || min > 59)
{
//Reject;
return;
}
else if (end_ptr == tm || *end_ptr != '\0')
{
//Reject
return;
}
else
{
errno = 0;

@ strcpy(tm, strtok(NULL,":"));

sec = strtol(tm, &end_ptr, 10);

if (sec > INT_MAX || sec < INT_MIN || sec > 59)
{
//Reject;
return;
}
else if (end_ptr == tm || *end_ptr != '\0')
{
//Reject
return;
}
else
{
sec_duration = hr * 3600 + min * 60 + sec;
printf("duration in seconds=%d\n",(int)sec_duration);
}
}
}
}
}

 

[Bart van Ingen Schenau]

[C application running on TRU64 Unix]

Hello All

I have a simple task that is driving me crazy. A string representing a
duration in the following format is passed to my application, a
function is dedicated to convert this duration to seconds;

[H]H:MM:SS

e.g. 0:00:00 or 00:12:45

The original function used atoi as part of the conversion process and
it produced some scary conversions 0:00:10 converted to 956 seconds
etc.

The function was rewritten to make use of strtol because of its better
error handling functionality but it now core dumps! Do someone have a
bullet proof idea to do this conversion assuming that anything might
be passed into this function;

e.g. A:00:00 or :0:45 etc

Reproducted below is the offending function the lines where it
currently dumps is marked with a @.

From reading your code, these invalid time strings should be rejected.
As the format is rather strict in what you can accept, I would use
sscanf() for this task.

void secondDurations()
{
int hr, min, sec, num_matches, num_chars;
long sec_duration = 0;
char duration[41];

// 00:00:00
// 0:00:00
// 01234567

strcpy(duration, "00:00:34");

//String should at leat be 7 characters long
if (strlen(duration) < 7)
{
// Reject
return;
}

/* The trailing %n causes the number of characters consumed to be
recorded. This should be the entire length of the string */
num_matches = sscanf(duration, "%d:%d:%d%n", &hr, &min, &sec,
&num_chars);
if ((num_matches != 3) || (num_chars != strlen(duration)))
{
// Malformed date. Reject
return;
}
else
{
sec_duration = hr * 3600 + min * 60 + sec;
printf("duration in seconds=%ld\n",sec_duration);
}
}

Any help would be much appreciated.

Thank you,

B

Bart v Ingen Schenau

 

[Fred Kleinschmidt]

[C application running on TRU64 Unix]

Hello All

I have a simple task that is driving me crazy. A string representing a
duration in the following format is passed to my application, a
function is dedicated to convert this duration to seconds;

[H]H:MM:SS

e.g. 0:00:00 or 00:12:45

The original function used atoi as part of the conversion process and
it produced some scary conversions 0:00:10 converted to 956 seconds
etc.

The function was rewritten to make use of strtol because of its better
error handling functionality but it now core dumps! Do someone have a
bullet proof idea to do this conversion assuming that anything might
be passed into this function;

e.g. A:00:00 or :0:45 etc

Reproducted below is the offending function the lines where it
currently dumps is marked with a @.

Any help would be much appreciated.

Thank you,

B

void secondDurations()
{
long hr, min, sec, sec_duration = 0;
char tm[10];
int ch = ':';
char *end_ptr = NULL;
char duration[41];

char *pfirst;
char *plast;

// 00:00:00
// 0:00:00
// 01234567


strcpy(duration, "00:00:34");

//String should at leat be 7 characters long
if (strlen(duration) >= 7)
{
pfirst = strchr(duration, ch);
plast = strrchr(duration, ch);

//Test duration format 00:00:00
if((pfirst != NULL || plast != NULL) && (pfirst != plast))
{
errno = 0;

//Get hour portion
strcpy(tm, strtok(duration,":"));

//Convert
hr = strtol(tm, &end_ptr, 10);

//Test
if (hr > INT_MAX || hr < INT_MIN)
{
//Reject;
return;
}
else if (end_ptr == tm || *end_ptr != '\0')
{
//Reject
return;
}
else
{
errno = 0;

@ strcpy(tm, strtok(NULL,":"));

min = strtol(tm, &end_ptr, 10);

if (min > INT_MAX || min < INT_MIN || min > 59)
{
//Reject;
return;
}
else if (end_ptr == tm || *end_ptr != '\0')
{
//Reject
return;
}
else
{
errno = 0;

@ strcpy(tm, strtok(NULL,":"));

sec = strtol(tm, &end_ptr, 10);

if (sec > INT_MAX || sec < INT_MIN || sec > 59)
{
//Reject;
return;
}
else if (end_ptr == tm || *end_ptr != '\0')
{
//Reject
return;
}
else
{
sec_duration = hr * 3600 + min * 60 + sec;
printf("duration in seconds=%d\n",(int)sec_duration);
}
}
}
}
}

Try something like this:
long hrs, mins, sec;
char *ptr, *next;
hrs = strtol( duration, *ptr, 10 );
if ( (*ptr == ':' ) && (ptr != duration) ) {
/* hours read, try minutes */
next = ptr+1;
mins = strtol( next, &ptr, 10 );
if ( (*ptr == ':') && (ptr != next) } {
next = ptr+1;
sec = strtol( next, &ptr, 10 );
if ( *ptr == '\0' ) {
/* good - compute total seconds */

}
}
}

Other safety chacks should be made, such as that
ptr-duration is 1 or 2, and ptr-next is 2, etc.,
and that the hrs/mins/sec values are in the correct range.

 

[braam.vanheerden]

Hello Bart and Fred

Thank you for you suggestions and comments, you have been very
helpful! I will definitely make use of your inputs.

B

 

[Keith Thompson]

[C application running on TRU64 Unix]
I have a simple task that is driving me crazy. A string representing a
duration in the following format is passed to my application, a
function is dedicated to convert this duration to seconds;

[H]H:MM:SS

e.g. 0:00:00 or 00:12:45

The original function used atoi as part of the conversion process and
it produced some scary conversions 0:00:10 converted to 956 seconds
etc.

[...]

From reading your code, these invalid time strings should be rejected.
As the format is rather strict in what you can accept, I would use
sscanf() for this task.

void secondDurations()
{
int hr, min, sec, num_matches, num_chars; [...]
/* The trailing %n causes the number of characters consumed to be
recorded. This should be the entire length of the string */
num_matches = sscanf(duration, "%d:%d:%d%n", &hr, &min, &sec,
&num_chars);

[...]

If the string you're scanning contains a syntactically valid integer
value that can't be stored in an int, then sscanf with the "%d" format
invokes undefined behavior. This is true for all the numeric
conversion formats for the *scanf() functions.

If you want safety, you'll need to check for the number of digits
before invoking sscanf, or you'll need to use some other method (like
strtol).

I haven't studied the original code, so I don't know why it's blowing
up.

 

[Old Wolf]

[C application running on TRU64 Unix]
The original function used atoi as part of the conversion process and
it produced some scary conversions 0:00:10 converted to 956 seconds
etc.

The function was rewritten to make use of strtol because of its better
error handling functionality but it now core dumps! Do someone have a
bullet proof idea to do this conversion assuming that anything might
be passed into this function;

You forgot the following lines:
#include <stdio.h>
#include <string.h>
#include <errno.h>
#include <limits.h>

Once those were included, and an output line and a main function
added, the code compiles and runs fine for me.

You should have gotten many warnings when you compiled
the code -- if not, then find out how to turn up the
warning level on your compiler.

strcpy(duration, "00:00:34");

//String should at leat be 7 characters long
if (strlen(duration) >= 7)
{

You have a weird code structure here. It goes:
if (A)
{
if (B)
return;
else if (C)
return;
else
{
if (D)
return;
else if (E)
return;
else
{
/* dozens of lines */
printf(......);
}
}
/* end of function */

It is hard to follow. IMHO, much clearer is:
if ( !A )
return;
if ( B )
return;
if ( C )
return;
if ( D )
return;
if ( E )
return;
/* dozens of lines */
/* print results */

//Get hour portion
strcpy(tm, strtok(duration,":"));

strtok would return NULL if the stirng contains no colon.
As posted, this can't happen, but what if somebody changes
the 'ch' variable but does not update the strtok calls?
You should use the same variable for both.

Also, if the duration string contains a lot of characters
between the colons, you cause a buffer overflow when copying
into tm.

What is the purpose of this "tm" ? You could have
written:
hr = strtol( strtok(NULL, ":"), &end_ptr, 10 );

although in either case I would prefer that the result
of strtok be stored in an intermediate variable and
checked that it is not NULL.

 

[Barry Schwarz]

snip explanation

void secondDurations()
{
long hr, min, sec, sec_duration = 0;
char tm[10];
int ch = ':';
char *end_ptr = NULL;
char duration[41];

char *pfirst;
char *plast;

// 00:00:00
// 0:00:00
// 01234567


strcpy(duration, "00:00:34");

//String should at leat be 7 characters long
if (strlen(duration) >= 7)
{
pfirst = strchr(duration, ch);
plast = strrchr(duration, ch);

Is it possible for pfirst to be different from plast? Can these two
statements ever produce different results?

//Test duration format 00:00:00
if((pfirst != NULL || plast != NULL) && (pfirst != plast))
{
errno = 0;

//Get hour portion
strcpy(tm, strtok(duration,":"));

//Convert
hr = strtol(tm, &end_ptr, 10);

//Test
if (hr > INT_MAX || hr < INT_MIN)

On some systems, sizeof(long) is the same as sizeof(int).

{
//Reject;
return;
}
else if (end_ptr == tm || *end_ptr != '\0')
{
//Reject

You set errno to 0. Did you intend to set it to some other value now?

return;
}
else
{
errno = 0;

@ strcpy(tm, strtok(NULL,":"));

This will allow a string of the form 01::23:45 to be treated as
correct.

min = strtol(tm, &end_ptr, 10);

if (min > INT_MAX || min < INT_MIN || min > 59)
{
//Reject;
return;
}
else if (end_ptr == tm || *end_ptr != '\0')
{
//Reject
return;
}
else
{
errno = 0;

You only need to set it to 0 once. It can't become any more zero than
it is.

@ strcpy(tm, strtok(NULL,":"));

sec = strtol(tm, &end_ptr, 10);

if (sec > INT_MAX || sec < INT_MIN || sec > 59)
{
//Reject;
return;
}
else if (end_ptr == tm || *end_ptr != '\0')
{
//Reject
return;
}
else
{
sec_duration = hr * 3600 + min * 60 + sec;
printf("duration in seconds=%d\n",(int)sec_duration);
}
}
}
}
}

Remove del for email

 

[Barry Schwarz]

snip explanation

void secondDurations()
{
long hr, min, sec, sec_duration = 0;
char tm[10];
int ch = ':';
char *end_ptr = NULL;
char duration[41];

char *pfirst;
char *plast;

// 00:00:00
// 0:00:00
// 01234567


strcpy(duration, "00:00:34");

//String should at leat be 7 characters long
if (strlen(duration) >= 7)
{
pfirst = strchr(duration, ch);
plast = strrchr(duration, ch);

Is it possible for pfirst to be different from plast? Can these two
statements ever produce different results?

Obviously they can if I would only notice the fourth r in the second
statement.


Remove del for email

 

[CBFalconer]

.... snip ...

It is hard to follow. IMHO, much clearer is:
if ( !A )
return;
if ( B )
return;
if ( C )
return;
if ( D )
return;
if ( E )
return;
/* dozens of lines */
/* print results */

I would prefer:

if (!(!A || B || C || D || E)) {
/* dozens of lines */
/* print results */
}
return;

 

[jaysome]

I would prefer:

if (!(!A || B || C || D || E)) {
/* dozens of lines */
/* print results */
}
return;

Applying De Morgan's Theorem to your expression I get:

if ( A && !B && !C && !D && !E )
{
/* dozens of lines */
/* print results */
}

This is clearer to me, and it ostensibly takes advantage of C's
short-circuit evaluation of conditions to determine the value of the
expression. It's unclear to me whether your expression takes advantage
of C's short-circuit evaluation of conditions, given that everything
in the inner-most parenthesis is surrounded by a '!'.

Whether it is clearer to you or anyone else is a subjective matter.
Nevertheless, it's useful to apply De Morgan's Theorem to any
non-trivial expression as it gives you a pair of alternatives that
often makes it clear which one is the best, in terms of readability
and possibly even performance.

 

[Richard Bos]

I have a simple task that is driving me crazy. A string representing a
duration in the following format is passed to my application, a
function is dedicated to convert this duration to seconds;

[H]H:MM:SS

e.g. 0:00:00 or 00:12:45

As the format is rather strict in what you can accept, I would use
sscanf() for this task.

void secondDurations()
{
int hr, min, sec, num_matches, num_chars; [...]
/* The trailing %n causes the number of characters consumed to be
recorded. This should be the entire length of the string */
num_matches = sscanf(duration, "%d:%d:%d%n", &hr, &min, &sec,
&num_chars);

If the string you're scanning contains a syntactically valid integer
value that can't be stored in an int, then sscanf with the "%d" format
invokes undefined behavior. This is true for all the numeric
conversion formats for the *scanf() functions.

If you want safety, you'll need to check for the number of digits
before invoking sscanf, or you'll need to use some other method (like
strtol).

That's why his function carefully checks that the length of the _whole_
string is less than 7. Since INT_MAX can be 32767, that still isn't
careful enough, but it's some way towards the right amount of care.
Using longs instead of ints would solve the problem.

Richard

 

[Charlie Gordon]

strtok would return NULL if the stirng contains no colon.
As posted, this can't happen, but what if somebody changes
the 'ch' variable but does not update the strtok calls?
You should use the same variable for both.

Also, if the duration string contains a lot of characters
between the colons, you cause a buffer overflow when copying
into tm.

What is the purpose of this "tm" ? You could have
written:
hr = strtol( strtok(NULL, ":"), &end_ptr, 10 );

although in either case I would prefer that the result
of strtok be stored in an intermediate variable and
checked that it is not NULL.

Do not use strtok: it is non reentrant. It used a private global state
variable. It is error prone even in single threaded programs. A reentrant
version strtok_r is available on POSIX systems, but you hardly need
something to heavy for a simple parsing task such as this one. Here is a
program that performs the needed conversion, feel free to copy the relevant
function.

#include <stdio.h>
#include <ctype.h>

/* Convert a timestamp to a number of seconds:
* format is H:MM:SS or HH:MM:SS
* hour must be in range [0..23]
* minutes and seconds must be in range [0..59]
* return -1 in case of NULL pointer or invalid format
*/
long convert_time(const char *str) {
int hour, min, sec;

if (!str || !isdigit(*str))
return -1;
hour = *str++ - '0';
if (isdigit(*str))
hour = hour * 10 + (*str++ - '0');
if (*str != ':' || !isdigit(str[1]) || !isdigit(str[2]))
return -1;
min = (str[1] - '0') * 10 + (str[2] - '0');
str += 3;
if (*str != ':' || !isdigit(str[1]) || !isdigit(str[2]))
return -1;
sec = (str[1] - '0') * 10 + (str[2] - '0');
str += 3;
/* here you can relax the check on what can follow the time stamp */
if (*str != '\0')
return -1;
/* here you can customize the checks on timestamp validity */
if (hour > 23 || min > 59 || sec > 59)
return -1;
return hour * 3600L + min * 60 + sec;
}

int main(int argc, char **argv) {
int i;
for (i = 1; i < argc; i++) {
printf("\"%s\" -> %ld\n", argv, convert_time(argv));
}
return 0;
}

 

[Richard]

I would prefer:

if (!(!A || B || C || D || E)) {
/* dozens of lines */
/* print results */
}
return;

much clearer is

if (B || ... || (!A))
return

dozens of lines.

 

[Richard Bos]

much clearer is

if (B || ... || (!A))
return

That may be, but it doesn't have the same semantics, because || is a
short-circuiting operator.

Richard

 

[Richard]

That may be, but it doesn't have the same semantics, because || is a
short-circuiting operator.

Richard

True. I neglected to consider that A,B etc could be expressions/function
calls. restore the order.

The main point is I like the early exit. I always find that more
intuitive when debugging and not having to jump over code or let my eyes
roam to find the return.

IOW, if I shouldn't be there, get out.

 

[Richard Bos]

True. I neglected to consider that A,B etc could be expressions/function
calls. restore the order.

The main point is I like the early exit. I always find that more
intuitive when debugging and not having to jump over code or let my eyes
roam to find the return.

That, to me, depends on whether A, B, C and so on are true error
conditions or normal logic flow decisions. I tend to bail out early for
things like "we got passed a null pointer" and late for "this customer's
deduction works out to zero".

Richard

 

[CBFalconer]

.... snip long coded sample ...

Applying De Morgan's Theorem to your expression I get:

if ( A && !B && !C && !D && !E )
{
/* dozens of lines */
/* print results */
}

This is clearer to me, and it ostensibly takes advantage of C's
short-circuit evaluation of conditions to determine the value of
the expression. It's unclear to me whether your expression takes
advantage of C's short-circuit evaluation of conditions, given that
everything in the inner-most parenthesis is surrounded by a '!'.

While true enough, also consider the effort of evaluating the
conditional (although many optimizers will eliminate the
difference). With my statement, the decision to print is reached
after chacking a minimum number of components, while your 'reduced'
version requires all components to be checked. Of course the
reverse applies if optimizing for the 'skip it all' case.

(and yes, the short circuit operation functions in both)

At any rate my point was to eliminate the long confusing barrage of
lines in the original.

 

[Ben Bacarisse]

[C application running on TRU64 Unix]
I have a simple task that is driving me crazy. A string representing a
duration in the following format is passed to my application, a
function is dedicated to convert this duration to seconds;

[H]H:MM:SS

e.g. 0:00:00 or 00:12:45

The original function used atoi as part of the conversion process and
it produced some scary conversions 0:00:10 converted to 956 seconds
etc.

[...]

From reading your code, these invalid time strings should be rejected.
As the format is rather strict in what you can accept, I would use
sscanf() for this task.

void secondDurations()
{
int hr, min, sec, num_matches, num_chars; [...]
/* The trailing %n causes the number of characters consumed to be
recorded. This should be the entire length of the string */
num_matches = sscanf(duration, "%d:%d:%d%n", &hr, &min, &sec,
&num_chars);

[...]

If the string you're scanning contains a syntactically valid integer
value that can't be stored in an int, then sscanf with the "%d" format
invokes undefined behavior. This is true for all the numeric
conversion formats for the *scanf() functions.

If you want safety, you'll need to check for the number of digits
before invoking sscanf, or you'll need to use some other method (like
strtol).

There is another option. One can limit the number of digits read.
Also, the OP can use a trick to ensure the last number is no more than
two digits long:

sscanf(duration, "%2d:%2d:%2d%c", &hr, &min, &sec, &end) == 3

means the data is valid. The test is for three not four conversions
because the final one failing indicates that the string ends after no
more that a two-digit number. I think this is safe from undefined
behaviour.

If the string might not end after the last digit one can do:

nc = sscanf(duration, "%2d:%2d:%2d%c", &hr, &min, &sec, %end);
if (nc == 3 || nc == 4 && !isdigit(end)) /* data OK */

(I think 'unsigned char end;' is the best type for this kind of thing.)