OS_TimerCreate() Unterminated String

[skliper]

ostimer.c maintains a static OS_timer_table, and a char name[OS_MAX_API_NAME] is an element of each table entry.

In OS_TimerCreate(), ln 298, '''the code could leave an unterminated string in OS_timer_table[i].name'''. And it appears as though all the other code is assuming it IS a properly-terminated string. Line 243 tests:
{{{
if (strlen(timer_name) > OS_MAX_API_NAME)
...return error value
}}}
and later
{{{
strncpy(OS_timer_table[possible_tid].name, timer_name, OS_MAX_API_NAME);
}}}
copies the string with OS_MAX_API_NAME length.

But if the timer_name argument is sized exactly OS_MAX_API_NAME+1 (including the terminating null) then it'll be copied over so that there is no terminating null in the table entry name.

To fix:

• The strlen if-test should account for the \0 in its length check
  {{{
  if (strlen(timer_name) > OS_MAX_API_NAME-1)
  }}}
• The documentation for this function should note the actual arg length limit with the null
• (nice to have) It isn't actually documented in osconfig.h whether the terminating string nulls are counted as part of the OS_MAX_* name and path limits. But it certainly does appear that the intent is that strings are properly terminated in the VxWorks OSAL. (Written clairty on that convention would have helped some.)

[skliper]

Imported from trac issue 65. Created by abrown4 on 2015-06-30T18:45:27, last modified: 2019-08-14T14:11:46

[skliper]

Trac comment by abrown4 on 2015-07-08 13:01:14:

Similar problem found in OS_TimerGetIdByName(), line 463.

[skliper]

Trac comment by abrown4 on 2015-07-08 16:45:25:

Fixed vxworks ostimer.c in OS_TimerGetIdByName() and OS_TimerCreate() with the
{{{
if (strlen(timer_name) >= OS_MAX_API_NAME)
}}}
convention, as with osapi.c.

Also checked for use of the OS_MAX_API_NAME in the other OSAL's, they look correct for ostimer.c

Noted the posix osapi.c does a few things a little differently internally... but it isn't immediately obvious that is is incorrect without additional inspection:
{{{
int32 OS_QueueCreate (uint32 *queue_id, const char *queue_name, uint32 queue_depth,
uint32 data_size, uint32 flags)
{
int i;
// <...snip...>
char name[OS_MAX_API_NAME * 2];
char process_id_string[OS_MAX_API_NAME+1];
}}}

[skliper]

Trac comment by abrown4 on 2015-07-08 16:50:23:

commit: [changeset:37dc473] Fixed ostimer.c to prevent unterminated strings, Track #88.
Built upon ostimer.c coverage tests on Track #45. Bumped unit tests to 100% coverage with logic change.

[skliper]

Trac comment by abrown4 on 2015-07-08 16:59:32:

Recommended documentation changes:

'''OSAL-Configuration-guide.pdf''', page 10, section 2.3.2 "Configure the OSAL Parameter File":
for OS_MAX_API_NAME
'''Currently reads''': ''"The maximum length for an individual file name while in the OSAL File API:"''
'''The osconfig.h''' has:''"The maxium length allowed for a object (task,queue....) name "''
'''Recommend:''' ''"The maxium length allowed for a object name (task, queue, etc.), including terminating null"''

'''OSAL Library API''':
OS_TimerCreate(), OS_TimerGetIdByName(): both should reference OS_MAX_API_NAME for what is "too long" of a name and note the terminating null is included.

In '''osconfig.h''', recommend: ''"The maxium length allowed for a object name (task, queue, etc.), including terminating null"''

Also recommend looking at: OS_MAX_PATH_LEN and OS_BUFFER_SIZE, which involve string handling.

[skliper]

Trac comment by abrown4 on 2015-11-25 11:38:37:

Traceability: the above commit: [changeset:37dc473] has been picked up in #135, [changeset:093359f]. However, documentation changes are still recommended.

[skliper]

Trac comment by jhageman on 2019-02-28 15:01:33:

[changeset:093359f] does not include the unit test update in [changeset:37dc473], was this intended or should the unit test be updated (and run in an IC branch to confirm success) prior to closure of this issue?

[skliper]

Trac comment by jhageman on 2019-06-03 15:05:40:

Still needs documentation and possible unit test update.