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
backup",
                                 __func__);
                        return(-1);
                }
                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