Skip to content

Commit

Permalink
Fix iterator in hash_remove_current() to not skip elements.
Browse files Browse the repository at this point in the history
When iterator->current is NULL, hash_next() assumes we've reached the
end of a bucket (linked list) and moves to the next one. Wehn the first
element of a linked list was removed in hash_remove_current()
iterator->current was set to NULL, causing the next call to hash_next()
to skip over the rest of the list of that bucket.

To fix this we now decrement iterator->bucket by one, so the next call
to hash_next() correctly arrives at the new first element of the same
bucket. Doing it this way avoids having to search backwards through the
table to find the actual previous item.

This caused modules to be skipped in module_init_post_boot_device()
when normalizing module image paths so some of the module images ended
up non-normalized. This could then cause images to be loaded a second
time for modules that were part of an actually already loaded image.
This setup is present for the PCI module with the pci/x86 submodule
and would lead to a second copy of the PCI module image to be loaded
but without being initialized, eventually leading to #8684.

The affected module images were pretty much random, as it depended on
the order in which they were loaded from the file system, in this case
the boot floppy archive of the El-Torito boot part of ISO and anyboot
images. The r1alpha4 release images unfortunately had the module files
ordered in the archive just so that the PCI module image would be
skipped, allowing #8684 to happen on many systems with MSI support.

Since the block cache uses hash_remove_current() as well in some cases,
it is possible that transactions in its list could've been skipped.
Cursory testing didn't reveal this to be a usual case, and it is
possible that in the pattern it is used there, the bug wouldn't be
triggered at all. It's still possible that it caused rare misbehaviour
though.
  • Loading branch information
mmlr committed Nov 14, 2012
1 parent 1a6f1cb commit ad53cd2
Showing 1 changed file with 7 additions and 0 deletions.
7 changes: 7 additions & 0 deletions src/system/kernel/util/khash.cpp
Expand Up @@ -253,6 +253,13 @@ hash_remove_current(struct hash_table *table, struct hash_iterator *iterator)
} else {
table->table[index] = (struct hash_element *)NEXT(table,
element);

// We need to rewind the bucket, as hash_next() advances to the
// next bucket when iterator->current is NULL. With this we
// basically move the iterator between the end of the last
// bucket and before the start of this one so hash_next()
// doesn't skip the rest of this bucket.
iterator->bucket--;
}

table->num_elements--;
Expand Down

0 comments on commit ad53cd2

Please sign in to comment.