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(duratio n) >= 7)
{
pfirst = strchr(duration , ch);
plast = strrchr(duratio n, 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("duratio n in seconds=%d\n",( int)sec_duratio n);
}
}
}
}
}

[Richard Bos]

Keith Thompson <ks***@mib.orgw rote:

Bart van Ingen Schenau <ba**@ingen.ddn s.infowrites:

bcpkh wrote:

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]

"Old Wolf" <ol*****@inspir e.net.nza écrit dans le message de news:
11************* *********@19g20 00...legro ups.com...

On Sep 14, 3:25 am, bcpkh <vanheerden.br. [e-mail] te:

[...]

> //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.

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(co nst 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[i], convert_time(ar gv[i]));
}
return 0;
}
--
Chqrlie.

[Richard]

CBFalconer <cb********@yah oo.comwrites:

Old Wolf wrote:

>>

... 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;

much clearer is

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

dozens of lines.

>
--
Chuck F (cbfalconer at maineline dot net)
Available for consulting/temporary embedded and systems.
<http://cbfalconer.home .att.net>

[Richard Bos]

Richard <rg****@gmail.c omwrote:

CBFalconer <cb********@yah oo.comwrites:

I would prefer:

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

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

[CBFalconer]

jaysome wrote:

CBFalconer <cb********@yah oo.comwrote:

.... snip long coded sample ...

>

> 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 '!'.

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.

--
Chuck F (cbfalconer at maineline dot net)
Available for consulting/temporary embedded and systems.
<http://cbfalconer.home .att.net>

--
Posted via a free Usenet account from http://www.teranews.com

[Ben Bacarisse]

Keith Thompson <ks***@mib.orgw rites:

Bart van Ingen Schenau <ba**@ingen.ddn s.infowrites:

>bcpkh wrote:

>>[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.)

--
Ben.