Messages in this thread | | | Date | Mon, 17 Oct 2016 13:02:18 +0200 | From | Boris Brezillon <> | Subject | Re: [PATCH] UBI: Fix crash in try_recover_peb() |
| |
On Mon, 17 Oct 2016 10:00:25 +0200 Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Boris, > > On Thu, Oct 13, 2016 at 4:24 PM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > On Thu, 13 Oct 2016 16:05:36 +0200 > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > >> drivers/mtd/ubi/eba.c: In function ‘try_recover_peb’: > >> drivers/mtd/ubi/eba.c:744: warning: ‘vid_hdr’ is used uninitialized in this function > >> > >> The pointer vid_hdr is indeed not initialized, leading to a crash when > >> it is dereferenced. > >> > >> Fix this by obtaining the pointer from the VID buffer, like is done > >> everywhere else. > > > > Indeed, I don't know how I missed that one :-/. > > > >> > >> Fixes: 3291b52f9ff0acc8 ("UBI: introduce the VID buffer concept") > >> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > > One minor comment below, otherwise > > > > Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > Thanks! > > >> --- > >> Completely untested. And I know nothing about UBI ;-) > >> --- > >> drivers/mtd/ubi/eba.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c > >> index 95c4048a371e87b6..388e46be6ad92805 100644 > >> --- a/drivers/mtd/ubi/eba.c > >> +++ b/drivers/mtd/ubi/eba.c > >> @@ -741,6 +741,7 @@ static int try_recover_peb(struct ubi_volume *vol, int pnum, int lnum, > >> goto out_put; > >> } > >> > >> + vid_hdr = ubi_get_vid_hdr(vidb); > > > > Can you move this assignment at variable declaration time? > > I can do that. However, that moves the call to ubi_get_vid_hdr() _before_ > the call to ubi_io_read_vid_hdr(). > > While that would still work (ubi_get_vid_hdr() would just return a pointer to > the not-yet-read data), I think it's better to not move it, to prevent people > from accidentally trying to use it before the data has been read.
Well, maybe. I just thought it would be safer to assign it as early as possible to avoid another 'uninitialized var' bug if move code around again (which will happen soon ;-)).
Anyway, it's not that important, I'll let Richard take the version he prefers.
Thanks again for the fix.
Boris
| |