* [PATCH] progress: Check for NULL filename @ 2015-08-12 15:53 dann frazier 2015-08-13 7:52 ` Andrei Borzenkov 0 siblings, 1 reply; 9+ messages in thread From: dann frazier @ 2015-08-12 15:53 UTC (permalink / raw) To: grub-devel Avoid a NULL pointer dereference if the upper fs layer hasn't set the file->name field. Files opened through the grub_net_fs interface currently do not have this field set (though perhaps they should?). Signed-off-by: dann frazier <dann.frazier@canonical.com> --- grub-core/lib/progress.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c index 63a0767..2775554 100644 --- a/grub-core/lib/progress.c +++ b/grub-core/lib/progress.c @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), percent = grub_divmod64 (100 * file->progress_offset, file->size, 0); - partial_file_name = grub_strrchr (file->name, '/'); - if (partial_file_name) + if (file->name && (partial_file_name = grub_strrchr (file->name, '/'))) partial_file_name++; else partial_file_name = ""; -- 2.5.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] progress: Check for NULL filename 2015-08-12 15:53 [PATCH] progress: Check for NULL filename dann frazier @ 2015-08-13 7:52 ` Andrei Borzenkov 2015-08-13 21:04 ` dann frazier 0 siblings, 1 reply; 9+ messages in thread From: Andrei Borzenkov @ 2015-08-13 7:52 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Dann Frazier On Wed, Aug 12, 2015 at 6:53 PM, dann frazier <dann.frazier@canonical.com> wrote: > Avoid a NULL pointer dereference if the upper fs layer hasn't set the > file->name field. Files opened through the grub_net_fs interface currently do > not have this field set (though perhaps they should?). > file->name is set in grub_file_open independently of any filesystem used. How comes it becomes empty? Do you see it in current GIT master? > Signed-off-by: dann frazier <dann.frazier@canonical.com> > --- > grub-core/lib/progress.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c > index 63a0767..2775554 100644 > --- a/grub-core/lib/progress.c > +++ b/grub-core/lib/progress.c > @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), > percent = grub_divmod64 (100 * file->progress_offset, > file->size, 0); > > - partial_file_name = grub_strrchr (file->name, '/'); > - if (partial_file_name) > + if (file->name && (partial_file_name = grub_strrchr (file->name, '/'))) > partial_file_name++; > else > partial_file_name = ""; > -- > 2.5.0 > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] progress: Check for NULL filename 2015-08-13 7:52 ` Andrei Borzenkov @ 2015-08-13 21:04 ` dann frazier 2015-08-17 18:49 ` Andrei Borzenkov 0 siblings, 1 reply; 9+ messages in thread From: dann frazier @ 2015-08-13 21:04 UTC (permalink / raw) To: Andrei Borzenkov; +Cc: The development of GNU GRUB On Thu, Aug 13, 2015 at 10:52:19AM +0300, Andrei Borzenkov wrote: > On Wed, Aug 12, 2015 at 6:53 PM, dann frazier > <dann.frazier@canonical.com> wrote: > > Avoid a NULL pointer dereference if the upper fs layer hasn't set the > > file->name field. Files opened through the grub_net_fs interface currently do > > not have this field set (though perhaps they should?). > > > > file->name is set in grub_file_open independently of any filesystem > used. How comes it becomes empty? Do you see it in current GIT > master? Yeah, I see it with current GIT master. Here's what I believe is happening. grub_file_open() calls the fs->open callback, *before* it initializes file->name. In the net_fs open callback (haven't checked others), it makes a copy of the file structure and instantiates a bufio file structure for it. It copies the bufio structure over the file structure that was passed in. Now we return to grub_file_open and set file->name in the (now bufio) file structure. But the original file structure backing the bufio still has a NULL name. When this bufio is read, it calls the read method on this backing file, which causes the progress hook to run and fall over. Perhaps the fix here is just to make grub_file_open set file->name before the fs_open callback? diff --git a/grub-core/kern/file.c b/grub-core/kern/file.c index 24da12b..4afa8c2 100644 --- a/grub-core/kern/file.c +++ b/grub-core/kern/file.c @@ -99,10 +99,11 @@ grub_file_open (const char *name) goto fail; } + file->name = grub_strdup (name); + if ((file->fs->open) (file, file_name) != GRUB_ERR_NONE) goto fail; - file->name = grub_strdup (name); grub_errno = GRUB_ERR_NONE; for (filter = 0; file && filter < ARRAY_SIZE (grub_file_filters_enabled); @@ -126,6 +127,7 @@ grub_file_open (const char *name) /* if (net) grub_net_close (net); */ + grub_free (file->name); grub_free (file); grub_memcpy (grub_file_filters_enabled, grub_file_filters_all, > > > Signed-off-by: dann frazier <dann.frazier@canonical.com> > > --- > > grub-core/lib/progress.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c > > index 63a0767..2775554 100644 > > --- a/grub-core/lib/progress.c > > +++ b/grub-core/lib/progress.c > > @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), > > percent = grub_divmod64 (100 * file->progress_offset, > > file->size, 0); > > > > - partial_file_name = grub_strrchr (file->name, '/'); > > - if (partial_file_name) > > + if (file->name && (partial_file_name = grub_strrchr (file->name, '/'))) > > partial_file_name++; > > else > > partial_file_name = ""; > > > > > > _______________________________________________ > > Grub-devel mailing list > > Grub-devel@gnu.org > > https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] progress: Check for NULL filename 2015-08-13 21:04 ` dann frazier @ 2015-08-17 18:49 ` Andrei Borzenkov 2015-08-17 21:57 ` [PATCH] Avoid NULL pointer dereference in progress module dann frazier 0 siblings, 1 reply; 9+ messages in thread From: Andrei Borzenkov @ 2015-08-17 18:49 UTC (permalink / raw) To: dann frazier; +Cc: The development of GNU GRUB 14.08.2015 00:04, dann frazier пишет: > On Thu, Aug 13, 2015 at 10:52:19AM +0300, Andrei Borzenkov wrote: >> On Wed, Aug 12, 2015 at 6:53 PM, dann frazier >> <dann.frazier@canonical.com> wrote: >>> Avoid a NULL pointer dereference if the upper fs layer hasn't set the >>> file->name field. Files opened through the grub_net_fs interface currently do >>> not have this field set (though perhaps they should?). >>> >> >> file->name is set in grub_file_open independently of any filesystem >> used. How comes it becomes empty? Do you see it in current GIT >> master? > > Yeah, I see it with current GIT master. Here's what I believe is happening. > > grub_file_open() calls the fs->open callback, *before* it initializes > file->name. In the net_fs open callback (haven't checked others), it > makes a copy of the file structure and instantiates a bufio file > structure for it. It copies the bufio structure over the file > structure that was passed in. > > Now we return to grub_file_open and set file->name in the (now bufio) > file structure. But the original file structure backing the bufio > still has a NULL name. When this bufio is read, it calls the read > method on this backing file, which causes the progress hook to run and > fall over. > > Perhaps the fix here is just to make grub_file_open set file->name > before the fs_open callback? > Then grub_net_fs_open needs to duplicate file->name, otherwise it will result in double free of file->name (first by grub_file_close in grub_bufio_close and then by original grub_file_close). And grub_file_open ignores NULL return from grub_strdup so we need check in progress anyway. I wish we could simply use grub_buffile_open, but I do not see easy way. Could you make combined patch? > diff --git a/grub-core/kern/file.c b/grub-core/kern/file.c > index 24da12b..4afa8c2 100644 > --- a/grub-core/kern/file.c > +++ b/grub-core/kern/file.c > @@ -99,10 +99,11 @@ grub_file_open (const char *name) > goto fail; > } > > + file->name = grub_strdup (name); > + > if ((file->fs->open) (file, file_name) != GRUB_ERR_NONE) > goto fail; > > - file->name = grub_strdup (name); > grub_errno = GRUB_ERR_NONE; > > for (filter = 0; file && filter < ARRAY_SIZE (grub_file_filters_enabled); > @@ -126,6 +127,7 @@ grub_file_open (const char *name) > > /* if (net) grub_net_close (net); */ > > + grub_free (file->name); > grub_free (file); > > grub_memcpy (grub_file_filters_enabled, grub_file_filters_all, > > >> >>> Signed-off-by: dann frazier <dann.frazier@canonical.com> >>> --- >>> grub-core/lib/progress.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c >>> index 63a0767..2775554 100644 >>> --- a/grub-core/lib/progress.c >>> +++ b/grub-core/lib/progress.c >>> @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), >>> percent = grub_divmod64 (100 * file->progress_offset, >>> file->size, 0); >>> >>> - partial_file_name = grub_strrchr (file->name, '/'); >>> - if (partial_file_name) >>> + if (file->name && (partial_file_name = grub_strrchr (file->name, '/'))) >>> partial_file_name++; >>> else >>> partial_file_name = ""; >>> >>> >>> _______________________________________________ >>> Grub-devel mailing list >>> Grub-devel@gnu.org >>> https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] Avoid NULL pointer dereference in progress module. 2015-08-17 18:49 ` Andrei Borzenkov @ 2015-08-17 21:57 ` dann frazier 2015-08-20 17:55 ` Andrei Borzenkov 0 siblings, 1 reply; 9+ messages in thread From: dann frazier @ 2015-08-17 21:57 UTC (permalink / raw) To: Andrei Borzenkov; +Cc: The development of GNU GRUB grub_net_fs_open() saves off a copy of the file structure it gets passed and uses it to create a bufio structure. It then overwrites the passed in file structure with this new bufio structure. Since file->name doesn't get set until we return back to grub_file_open(), it means that only the bufio structure gets a valid file->name. The "real" file's name is left uninitialized. This leads to a crash when the progress module hook is called on it. Fix this by adding a copy of the filename during the switcheroo. grub_file_close() will free that string in the success case, we only need to handle the free in an error. While we're at it, also fix a leaked file structure in the case that grub_strdup() fails when setting file->device->net->name. Finally, make progress more robust by checking for NULL before reading the name. Andrei noticed that this could occur if grub_strdup() fails in grub_file_open(). Signed-off-by: dann frazier <dann.frazier@canonical.com> --- grub-core/lib/progress.c | 3 +-- grub-core/net/net.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c index 63a0767..2775554 100644 --- a/grub-core/lib/progress.c +++ b/grub-core/lib/progress.c @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), percent = grub_divmod64 (100 * file->progress_offset, file->size, 0); - partial_file_name = grub_strrchr (file->name, '/'); - if (partial_file_name) + if (file->name && (partial_file_name = grub_strrchr (file->name, '/'))) partial_file_name++; else partial_file_name = ""; diff --git a/grub-core/net/net.c b/grub-core/net/net.c index 21a4e94..9f83765 100644 --- a/grub-core/net/net.c +++ b/grub-core/net/net.c @@ -1391,7 +1391,17 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) file->device->net->packs.last = NULL; file->device->net->name = grub_strdup (name); if (!file->device->net->name) - return grub_errno; + { + grub_free (file); + return grub_errno; + } + file->name = grub_strdup (name); + if (!file->name) + { + grub_free (file->device->net->name); + grub_free (file); + return grub_errno; + } err = file->device->net->protocol->open (file, name); if (err) @@ -1401,6 +1411,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) grub_netbuff_free (file->device->net->packs.first->nb); grub_net_remove_packet (file->device->net->packs.first); } + grub_free (file->name); grub_free (file->device->net->name); grub_free (file); return err; @@ -1414,6 +1425,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) grub_net_remove_packet (file->device->net->packs.first); } file->device->net->protocol->close (file); + grub_free (file->name); grub_free (file->device->net->name); grub_free (file); return grub_errno; -- 2.5.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Avoid NULL pointer dereference in progress module. 2015-08-17 21:57 ` [PATCH] Avoid NULL pointer dereference in progress module dann frazier @ 2015-08-20 17:55 ` Andrei Borzenkov 2015-08-21 14:24 ` Dann Frazier 0 siblings, 1 reply; 9+ messages in thread From: Andrei Borzenkov @ 2015-08-20 17:55 UTC (permalink / raw) To: dann frazier; +Cc: The development of GNU GRUB [-- Attachment #1: Type: text/plain, Size: 3330 bytes --] 18.08.2015 00:57, dann frazier пишет: > grub_net_fs_open() saves off a copy of the file structure it gets passed and > uses it to create a bufio structure. It then overwrites the passed in file > structure with this new bufio structure. Since file->name doesn't get set > until we return back to grub_file_open(), it means that only the bufio > structure gets a valid file->name. The "real" file's name is left > uninitialized. This leads to a crash when the progress module hook is called > on it. Fix this by adding a copy of the filename during the switcheroo. Actually name was already saved off as device->net-name. What about attached patch? I'll commit leak fix separately. > grub_file_close() will free that string in the success case, we only need > to handle the free in an error. While we're at it, also fix a leaked file > structure in the case that grub_strdup() fails when setting > file->device->net->name. > > Finally, make progress more robust by checking for NULL before reading the > name. Andrei noticed that this could occur if grub_strdup() fails in > grub_file_open(). > > Signed-off-by: dann frazier <dann.frazier@canonical.com> > --- > grub-core/lib/progress.c | 3 +-- > grub-core/net/net.c | 14 +++++++++++++- > 2 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c > index 63a0767..2775554 100644 > --- a/grub-core/lib/progress.c > +++ b/grub-core/lib/progress.c > @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), > percent = grub_divmod64 (100 * file->progress_offset, > file->size, 0); > > - partial_file_name = grub_strrchr (file->name, '/'); > - if (partial_file_name) > + if (file->name && (partial_file_name = grub_strrchr (file->name, '/'))) > partial_file_name++; > else > partial_file_name = ""; > diff --git a/grub-core/net/net.c b/grub-core/net/net.c > index 21a4e94..9f83765 100644 > --- a/grub-core/net/net.c > +++ b/grub-core/net/net.c > @@ -1391,7 +1391,17 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) > file->device->net->packs.last = NULL; > file->device->net->name = grub_strdup (name); > if (!file->device->net->name) > - return grub_errno; > + { > + grub_free (file); > + return grub_errno; > + } > + file->name = grub_strdup (name); > + if (!file->name) > + { > + grub_free (file->device->net->name); > + grub_free (file); > + return grub_errno; > + } > > err = file->device->net->protocol->open (file, name); > if (err) > @@ -1401,6 +1411,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) > grub_netbuff_free (file->device->net->packs.first->nb); > grub_net_remove_packet (file->device->net->packs.first); > } > + grub_free (file->name); > grub_free (file->device->net->name); > grub_free (file); > return err; > @@ -1414,6 +1425,7 @@ grub_net_fs_open (struct grub_file *file_out, const char *name) > grub_net_remove_packet (file->device->net->packs.first); > } > file->device->net->protocol->close (file); > + grub_free (file->name); > grub_free (file->device->net->name); > grub_free (file); > return grub_errno; > [-- Attachment #2: proress-net.patch --] [-- Type: text/x-patch, Size: 1906 bytes --] From: Andrei Borzenkov <arvidjaar@gmail.com> Subject: [PATCH] progress: avoid NULL dereference for net files From original patch by dann frazier <dann.frazier@canonical.com>: grub_net_fs_open() saves off a copy of the file structure it gets passed and uses it to create a bufio structure. It then overwrites the passed in file structure with this new bufio structure. Since file->name doesn't get set until we return back to grub_file_open(), it means that only the bufio structure gets a valid file->name. The "real" file's name is left uninitialized. This leads to a crash when the progress module hook is called on it. grub_net_fs_open() already saved copy of file name as ->net->name, so change progress module to use it. Also, grub_file_open may leave file->name as NULL if grub_strdup fails. Check for it. Also-By: dann frazier <dann.frazier@canonical.com> --- grub-core/lib/progress.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c index 63a0767..7c723eb 100644 --- a/grub-core/lib/progress.c +++ b/grub-core/lib/progress.c @@ -23,6 +23,7 @@ #include <grub/dl.h> #include <grub/misc.h> #include <grub/normal.h> +#include <grub/net.h> GRUB_MOD_LICENSE ("GPLv3+"); @@ -70,7 +71,12 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)), percent = grub_divmod64 (100 * file->progress_offset, file->size, 0); - partial_file_name = grub_strrchr (file->name, '/'); + if (file->device->net) + partial_file_name = grub_strrchr (file->device->net->name, '/'); + else if (file->name) /* grub_file_open() may leave it as NULL */ + partial_file_name = grub_strrchr (file->name, '/'); + else + partial_file_name = NULL; if (partial_file_name) partial_file_name++; else -- tg: (ba218c1..) u/progress-net-file (depends on: master) ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Avoid NULL pointer dereference in progress module. 2015-08-20 17:55 ` Andrei Borzenkov @ 2015-08-21 14:24 ` Dann Frazier 2015-09-21 15:11 ` Dann Frazier 0 siblings, 1 reply; 9+ messages in thread From: Dann Frazier @ 2015-08-21 14:24 UTC (permalink / raw) To: Andrei Borzenkov; +Cc: The development of GNU GRUB On Thu, Aug 20, 2015 at 11:55 AM, Andrei Borzenkov <arvidjaar@gmail.com> wrote: > 18.08.2015 00:57, dann frazier пишет: >> >> grub_net_fs_open() saves off a copy of the file structure it gets passed >> and >> uses it to create a bufio structure. It then overwrites the passed in file >> structure with this new bufio structure. Since file->name doesn't get set >> until we return back to grub_file_open(), it means that only the bufio >> structure gets a valid file->name. The "real" file's name is left >> uninitialized. This leads to a crash when the progress module hook is >> called >> on it. Fix this by adding a copy of the filename during the switcheroo. > > > Actually name was already saved off as device->net-name. What about attached > patch? I'll commit leak fix separately. It does seem like it pokes a hole in the fs abstraction, but it works for me. -dann > >> grub_file_close() will free that string in the success case, we only need >> to handle the free in an error. While we're at it, also fix a leaked file >> structure in the case that grub_strdup() fails when setting >> file->device->net->name. >> >> Finally, make progress more robust by checking for NULL before reading the >> name. Andrei noticed that this could occur if grub_strdup() fails in >> grub_file_open(). >> >> Signed-off-by: dann frazier <dann.frazier@canonical.com> >> --- >> grub-core/lib/progress.c | 3 +-- >> grub-core/net/net.c | 14 +++++++++++++- >> 2 files changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c >> index 63a0767..2775554 100644 >> --- a/grub-core/lib/progress.c >> +++ b/grub-core/lib/progress.c >> @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector >> __attribute__ ((unused)), >> percent = grub_divmod64 (100 * file->progress_offset, >> file->size, 0); >> >> - partial_file_name = grub_strrchr (file->name, '/'); >> - if (partial_file_name) >> + if (file->name && (partial_file_name = grub_strrchr (file->name, >> '/'))) >> partial_file_name++; >> else >> partial_file_name = ""; >> diff --git a/grub-core/net/net.c b/grub-core/net/net.c >> index 21a4e94..9f83765 100644 >> --- a/grub-core/net/net.c >> +++ b/grub-core/net/net.c >> @@ -1391,7 +1391,17 @@ grub_net_fs_open (struct grub_file *file_out, const >> char *name) >> file->device->net->packs.last = NULL; >> file->device->net->name = grub_strdup (name); >> if (!file->device->net->name) >> - return grub_errno; >> + { >> + grub_free (file); >> + return grub_errno; >> + } >> + file->name = grub_strdup (name); >> + if (!file->name) >> + { >> + grub_free (file->device->net->name); >> + grub_free (file); >> + return grub_errno; >> + } >> >> err = file->device->net->protocol->open (file, name); >> if (err) >> @@ -1401,6 +1411,7 @@ grub_net_fs_open (struct grub_file *file_out, const >> char *name) >> grub_netbuff_free (file->device->net->packs.first->nb); >> grub_net_remove_packet (file->device->net->packs.first); >> } >> + grub_free (file->name); >> grub_free (file->device->net->name); >> grub_free (file); >> return err; >> @@ -1414,6 +1425,7 @@ grub_net_fs_open (struct grub_file *file_out, const >> char *name) >> grub_net_remove_packet (file->device->net->packs.first); >> } >> file->device->net->protocol->close (file); >> + grub_free (file->name); >> grub_free (file->device->net->name); >> grub_free (file); >> return grub_errno; >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Avoid NULL pointer dereference in progress module. 2015-08-21 14:24 ` Dann Frazier @ 2015-09-21 15:11 ` Dann Frazier 2015-10-10 8:44 ` Andrei Borzenkov 0 siblings, 1 reply; 9+ messages in thread From: Dann Frazier @ 2015-09-21 15:11 UTC (permalink / raw) To: Andrei Borzenkov; +Cc: The development of GNU GRUB On Fri, Aug 21, 2015 at 8:24 AM, Dann Frazier <dann.frazier@canonical.com> wrote: > On Thu, Aug 20, 2015 at 11:55 AM, Andrei Borzenkov <arvidjaar@gmail.com> wrote: >> 18.08.2015 00:57, dann frazier пишет: >>> >>> grub_net_fs_open() saves off a copy of the file structure it gets passed >>> and >>> uses it to create a bufio structure. It then overwrites the passed in file >>> structure with this new bufio structure. Since file->name doesn't get set >>> until we return back to grub_file_open(), it means that only the bufio >>> structure gets a valid file->name. The "real" file's name is left >>> uninitialized. This leads to a crash when the progress module hook is >>> called >>> on it. Fix this by adding a copy of the filename during the switcheroo. >> >> >> Actually name was already saved off as device->net-name. What about attached >> patch? I'll commit leak fix separately. > > It does seem like it pokes a hole in the fs abstraction, but it works for me. hey Andrei, Did you need anything more from me wrt this fix? -dann >> >>> grub_file_close() will free that string in the success case, we only need >>> to handle the free in an error. While we're at it, also fix a leaked file >>> structure in the case that grub_strdup() fails when setting >>> file->device->net->name. >>> >>> Finally, make progress more robust by checking for NULL before reading the >>> name. Andrei noticed that this could occur if grub_strdup() fails in >>> grub_file_open(). >>> >>> Signed-off-by: dann frazier <dann.frazier@canonical.com> >>> --- >>> grub-core/lib/progress.c | 3 +-- >>> grub-core/net/net.c | 14 +++++++++++++- >>> 2 files changed, 14 insertions(+), 3 deletions(-) >>> >>> diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c >>> index 63a0767..2775554 100644 >>> --- a/grub-core/lib/progress.c >>> +++ b/grub-core/lib/progress.c >>> @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector >>> __attribute__ ((unused)), >>> percent = grub_divmod64 (100 * file->progress_offset, >>> file->size, 0); >>> >>> - partial_file_name = grub_strrchr (file->name, '/'); >>> - if (partial_file_name) >>> + if (file->name && (partial_file_name = grub_strrchr (file->name, >>> '/'))) >>> partial_file_name++; >>> else >>> partial_file_name = ""; >>> diff --git a/grub-core/net/net.c b/grub-core/net/net.c >>> index 21a4e94..9f83765 100644 >>> --- a/grub-core/net/net.c >>> +++ b/grub-core/net/net.c >>> @@ -1391,7 +1391,17 @@ grub_net_fs_open (struct grub_file *file_out, const >>> char *name) >>> file->device->net->packs.last = NULL; >>> file->device->net->name = grub_strdup (name); >>> if (!file->device->net->name) >>> - return grub_errno; >>> + { >>> + grub_free (file); >>> + return grub_errno; >>> + } >>> + file->name = grub_strdup (name); >>> + if (!file->name) >>> + { >>> + grub_free (file->device->net->name); >>> + grub_free (file); >>> + return grub_errno; >>> + } >>> >>> err = file->device->net->protocol->open (file, name); >>> if (err) >>> @@ -1401,6 +1411,7 @@ grub_net_fs_open (struct grub_file *file_out, const >>> char *name) >>> grub_netbuff_free (file->device->net->packs.first->nb); >>> grub_net_remove_packet (file->device->net->packs.first); >>> } >>> + grub_free (file->name); >>> grub_free (file->device->net->name); >>> grub_free (file); >>> return err; >>> @@ -1414,6 +1425,7 @@ grub_net_fs_open (struct grub_file *file_out, const >>> char *name) >>> grub_net_remove_packet (file->device->net->packs.first); >>> } >>> file->device->net->protocol->close (file); >>> + grub_free (file->name); >>> grub_free (file->device->net->name); >>> grub_free (file); >>> return grub_errno; >>> >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Avoid NULL pointer dereference in progress module. 2015-09-21 15:11 ` Dann Frazier @ 2015-10-10 8:44 ` Andrei Borzenkov 0 siblings, 0 replies; 9+ messages in thread From: Andrei Borzenkov @ 2015-10-10 8:44 UTC (permalink / raw) To: Dann Frazier; +Cc: The development of GNU GRUB 21.09.2015 18:11, Dann Frazier пишет: > On Fri, Aug 21, 2015 at 8:24 AM, Dann Frazier > <dann.frazier@canonical.com> wrote: >> On Thu, Aug 20, 2015 at 11:55 AM, Andrei Borzenkov <arvidjaar@gmail.com> wrote: >>> 18.08.2015 00:57, dann frazier пишет: >>>> >>>> grub_net_fs_open() saves off a copy of the file structure it gets passed >>>> and >>>> uses it to create a bufio structure. It then overwrites the passed in file >>>> structure with this new bufio structure. Since file->name doesn't get set >>>> until we return back to grub_file_open(), it means that only the bufio >>>> structure gets a valid file->name. The "real" file's name is left >>>> uninitialized. This leads to a crash when the progress module hook is >>>> called >>>> on it. Fix this by adding a copy of the filename during the switcheroo. >>> >>> >>> Actually name was already saved off as device->net-name. What about attached >>> patch? I'll commit leak fix separately. >> >> It does seem like it pokes a hole in the fs abstraction, but it works for me. > > hey Andrei, > Did you need anything more from me wrt this fix? > Sorry for delay. We already need to differentiate between disk and net files in other places anyway. So I think it is OK as a simple fix. I committed it. May be bufio needs to be converted to filter but this is too intrusive for this. > -dann > >>> >>>> grub_file_close() will free that string in the success case, we only need >>>> to handle the free in an error. While we're at it, also fix a leaked file >>>> structure in the case that grub_strdup() fails when setting >>>> file->device->net->name. >>>> >>>> Finally, make progress more robust by checking for NULL before reading the >>>> name. Andrei noticed that this could occur if grub_strdup() fails in >>>> grub_file_open(). >>>> >>>> Signed-off-by: dann frazier <dann.frazier@canonical.com> >>>> --- >>>> grub-core/lib/progress.c | 3 +-- >>>> grub-core/net/net.c | 14 +++++++++++++- >>>> 2 files changed, 14 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c >>>> index 63a0767..2775554 100644 >>>> --- a/grub-core/lib/progress.c >>>> +++ b/grub-core/lib/progress.c >>>> @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector >>>> __attribute__ ((unused)), >>>> percent = grub_divmod64 (100 * file->progress_offset, >>>> file->size, 0); >>>> >>>> - partial_file_name = grub_strrchr (file->name, '/'); >>>> - if (partial_file_name) >>>> + if (file->name && (partial_file_name = grub_strrchr (file->name, >>>> '/'))) >>>> partial_file_name++; >>>> else >>>> partial_file_name = ""; >>>> diff --git a/grub-core/net/net.c b/grub-core/net/net.c >>>> index 21a4e94..9f83765 100644 >>>> --- a/grub-core/net/net.c >>>> +++ b/grub-core/net/net.c >>>> @@ -1391,7 +1391,17 @@ grub_net_fs_open (struct grub_file *file_out, const >>>> char *name) >>>> file->device->net->packs.last = NULL; >>>> file->device->net->name = grub_strdup (name); >>>> if (!file->device->net->name) >>>> - return grub_errno; >>>> + { >>>> + grub_free (file); >>>> + return grub_errno; >>>> + } >>>> + file->name = grub_strdup (name); >>>> + if (!file->name) >>>> + { >>>> + grub_free (file->device->net->name); >>>> + grub_free (file); >>>> + return grub_errno; >>>> + } >>>> >>>> err = file->device->net->protocol->open (file, name); >>>> if (err) >>>> @@ -1401,6 +1411,7 @@ grub_net_fs_open (struct grub_file *file_out, const >>>> char *name) >>>> grub_netbuff_free (file->device->net->packs.first->nb); >>>> grub_net_remove_packet (file->device->net->packs.first); >>>> } >>>> + grub_free (file->name); >>>> grub_free (file->device->net->name); >>>> grub_free (file); >>>> return err; >>>> @@ -1414,6 +1425,7 @@ grub_net_fs_open (struct grub_file *file_out, const >>>> char *name) >>>> grub_net_remove_packet (file->device->net->packs.first); >>>> } >>>> file->device->net->protocol->close (file); >>>> + grub_free (file->name); >>>> grub_free (file->device->net->name); >>>> grub_free (file); >>>> return grub_errno; >>>> >>> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-10-10 14:01 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-08-12 15:53 [PATCH] progress: Check for NULL filename dann frazier 2015-08-13 7:52 ` Andrei Borzenkov 2015-08-13 21:04 ` dann frazier 2015-08-17 18:49 ` Andrei Borzenkov 2015-08-17 21:57 ` [PATCH] Avoid NULL pointer dereference in progress module dann frazier 2015-08-20 17:55 ` Andrei Borzenkov 2015-08-21 14:24 ` Dann Frazier 2015-09-21 15:11 ` Dann Frazier 2015-10-10 8:44 ` Andrei Borzenkov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).