From 8ec5d9af446846f2087e6c44420283061c959b02 Mon Sep 17 00:00:00 2001 From: Jonas 'Sortie' Termansen Date: Sat, 6 Aug 2016 14:51:02 +0200 Subject: [PATCH] Fix linked list and shadowing bugs in kernel clock and timer code. --- kernel/clock.cpp | 37 +++++++++++++++++----------- kernel/include/sortix/kernel/timer.h | 12 ++++----- kernel/timer.cpp | 25 +++++++++++-------- 3 files changed, 43 insertions(+), 31 deletions(-) diff --git a/kernel/clock.cpp b/kernel/clock.cpp index 66eefeb9..f80b8300 100644 --- a/kernel/clock.cpp +++ b/kernel/clock.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2013 Jonas 'Sortie' Termansen. + * Copyright (c) 2013, 2016 Jonas 'Sortie' Termansen. * * Permission to use, copy, modify, and distribute this software for any * purpose with or without fee is hereby granted, provided that the above @@ -136,14 +136,14 @@ void Clock::RegisterAbsolute(Timer* timer) // Lock acquired. timer->flags |= TIMER_ACTIVE; Timer* before = NULL; - for ( Timer* iter = absolute_timer; iter; iter = before->next_timer ) + for ( Timer* iter = absolute_timer; iter; iter = iter->next_timer ) + { if ( timespec_lt(timer->value.it_value, iter->value.it_value) ) - break; - else before = iter; + } timer->prev_timer = before; - timer->next_timer = before ? before->next_timer : NULL; + timer->next_timer = before ? before->next_timer : absolute_timer; if ( timer->next_timer ) timer->next_timer->prev_timer = timer; (before ? before->next_timer : absolute_timer) = timer; } @@ -154,19 +154,22 @@ void Clock::RegisterDelay(Timer* timer) // Lock acquired. timer->flags |= TIMER_ACTIVE; Timer* before = NULL; - struct timespec before_time = timespec_nul(); - for ( Timer* iter = delay_timer; iter; iter = before->next_timer ) + for ( Timer* iter = delay_timer; iter; iter = iter->next_timer ) + { if ( timespec_lt(timer->value.it_value, iter->value.it_value) ) break; - else - before = iter, - before_time = timespec_add(before_time, iter->value.it_value); + timer->value.it_value = timespec_sub(timer->value.it_value, iter->value.it_value); + before = iter; + } - timer->value.it_value = timespec_sub(timer->value.it_value, before_time); timer->prev_timer = before; - timer->next_timer = before ? before->next_timer : NULL; - if ( timer->next_timer ) timer->next_timer->prev_timer = timer; - (before ? before->next_timer : delay_timer) = timer; + timer->next_timer = before ? before->next_timer : delay_timer; + if ( timer->next_timer ) + timer->next_timer->prev_timer = timer; + if ( before ) + before->next_timer = timer; + else + delay_timer = timer; if ( timer->next_timer ) timer->next_timer->value.it_value = @@ -183,6 +186,7 @@ void Clock::Register(Timer* timer) void Clock::UnlinkAbsolute(Timer* timer) // Lock acquired. { + assert(timer->flags & TIMER_ACTIVE); (timer->prev_timer ? timer->prev_timer->next_timer : absolute_timer) = timer->next_timer; if ( timer->next_timer ) timer->next_timer->prev_timer = timer->prev_timer; timer->prev_timer = timer->next_timer = NULL; @@ -191,6 +195,7 @@ void Clock::UnlinkAbsolute(Timer* timer) // Lock acquired. void Clock::UnlinkDelay(Timer* timer) // Lock acquired. { + assert(timer->flags & TIMER_ACTIVE); (timer->prev_timer ? timer->prev_timer->next_timer : delay_timer) = timer->next_timer; if ( timer->next_timer ) timer->next_timer->prev_timer = timer->prev_timer; if ( timer->next_timer ) timer->next_timer->value.it_value = timespec_add(timer->next_timer->value.it_value, timer->value.it_value); @@ -251,8 +256,10 @@ struct timespec Clock::SleepDelay(struct timespec duration) LockClock(); if ( !start_advancement_set ) - start_advancement = current_advancement, + { + start_advancement = current_advancement; start_advancement_set = true; + } elapsed = timespec_sub(current_advancement, start_advancement); diff --git a/kernel/include/sortix/kernel/timer.h b/kernel/include/sortix/kernel/timer.h index 29017fca..02125cd1 100644 --- a/kernel/include/sortix/kernel/timer.h +++ b/kernel/include/sortix/kernel/timer.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2013 Jonas 'Sortie' Termansen. + * Copyright (c) 2013, 2016 Jonas 'Sortie' Termansen. * * Permission to use, copy, modify, and distribute this software for any * purpose with or without fee is hereby granted, provided that the above @@ -30,11 +30,11 @@ namespace Sortix { class Clock; class Timer; -const int TIMER_ABSOLUTE = 1 << 0; -const int TIMER_ACTIVE = 1 << 1; -const int TIMER_FIRING = 1 << 2; -const int TIMER_FUNC_INTERRUPT_HANDLER = 1 << 3; -const int TIMER_FUNC_ADVANCE_THREAD = 1 << 4; +static const int TIMER_ABSOLUTE = 1 << 0; +static const int TIMER_ACTIVE = 1 << 1; +static const int TIMER_FIRING = 1 << 2; +static const int TIMER_FUNC_INTERRUPT_HANDLER = 1 << 3; +static const int TIMER_FUNC_ADVANCE_THREAD = 1 << 4; class Timer { diff --git a/kernel/timer.cpp b/kernel/timer.cpp index 587fdb4a..d854400a 100644 --- a/kernel/timer.cpp +++ b/kernel/timer.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2013 Jonas 'Sortie' Termansen. + * Copyright (c) 2013, 2016 Jonas 'Sortie' Termansen. * * Permission to use, copy, modify, and distribute this software for any * purpose with or without fee is hereby granted, provided that the above @@ -98,8 +98,9 @@ void Timer::Get(struct itimerspec* current) clock->UnlockClock(); } -void Timer::Set(struct itimerspec* value, struct itimerspec* ovalue, int flags, - void (*callback)(Clock*, Timer*, void*), void* user) +void Timer::Set(struct itimerspec* new_value, struct itimerspec* old_value, + int new_flags, void (*new_callback)(Clock*, Timer*, void*), + void* new_user) { assert(clock); @@ -107,19 +108,23 @@ void Timer::Set(struct itimerspec* value, struct itimerspec* ovalue, int flags, // Dequeue this timer if it is already armed. if ( flags & TIMER_ACTIVE ) + { + // TODO: How does this interplay with concurrently running timer + // handlers? Maybe the timer should be cancelled instead? clock->Unlink(this); + } // Let the caller know how much time was left on the timer. - if ( ovalue ) - GetInternal(ovalue); + if ( old_value ) + GetInternal(old_value); // Arm the timer if a value was specified. - if ( timespec_lt(timespec_nul(), value->it_value) ) + if ( timespec_lt(timespec_nul(), new_value->it_value) ) { - this->value = *value; - this->flags = flags; - this->callback = callback; - this->user = user; + value = *new_value; + flags = new_flags; + callback = new_callback; + user = new_user; clock->Register(this); }