possible bug in devstat_selectdevs()

From: Andriy Gapon <avg_at_FreeBSD.org>
Date: Wed, 13 Nov 2019 16:30:19 +0200
I wonder if anyone remembers devstat code enough to help me or, at least, to
sanity check my line of thinking.

I am looking at a crash that happened in devstat_selectdevs(num_selections=27,
numdevs=25).  At the time of the crash there was some reconfiguration of logical
volumes on a RAID controller, so "disks" were coming and going.

The first relevant block of code in the function is:
         * In this case, we have selected devices before, but the device
         * list has changed since we last selected devices, so we need to
         * either enlarge or reduce the size of the device selection list.
        } else if (*num_selections != numdevs) {
                *dev_select = (struct device_selection *)reallocf(*dev_select,
                        numdevs * sizeof(struct device_selection));
                *select_generation = current_generation;
                init_selections = 1;

So, dev_select array is realloc-ed to have space for numdevs elements.
Then we have this:
        if (((init_selected_var != 0) || (init_selections != 0)
         || (perf_select != 0)) && (changed == 0)){
                old_dev_select = (struct device_selection *)malloc(
                    *num_selections * sizeof(struct device_selection));
                if (old_dev_select == NULL) {
                        snprintf(devstat_errbuf, sizeof(devstat_errbuf),
                                 "%s: Cannot allocate memory for selection list
                old_num_selections = *num_selections;
==>             bcopy(*dev_select, old_dev_select,
                    sizeof(struct device_selection) * *num_selections);

The crash happened in the bcopy() call.
So, we are trying to copy num_selections (I omit pointer dereferencing) elements
from the dev_select array.  But in the previous block we resized the array to
numdevs and in this case numdevs is less than num_selections.

The code is quite unfamiliar to me.  My first instinct is to just clamp the copy
size, but I am not sure if that would be the right thing.
Maybe realloc of dev_select should be done after bcopy-ing out of the array?
Or maybe it's okay to realloc only if the size is going up?

Any help is appreciated.
Thank you very much in advance!
Andriy Gapon
Received on Wed Nov 13 2019 - 13:30:25 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:41:22 UTC