* [PATCH] fill_fs_info: pass pointer to dnode_end_t instead of value
@ 2022-08-19 14:57 Jagannathan Raman
2022-08-19 15:09 ` Darren Kenny
0 siblings, 1 reply; 4+ messages in thread
From: Jagannathan Raman @ 2022-08-19 14:57 UTC (permalink / raw)
To: grub-devel; +Cc: daniel.kiper, darren.kenny
Coverity reports that dnode_end_t argument of fill_fs_info() is too
large to pass-by-value. Therefore, replace the argument with a pointer.
Fixes: CID 73631
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
---
grub-core/fs/zfs/zfs.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
index ffa0e5863..975c67242 100644
--- a/grub-core/fs/zfs/zfs.c
+++ b/grub-core/fs/zfs/zfs.c
@@ -3983,14 +3983,23 @@ grub_zfs_getmdnobj (grub_device_t dev, const char *fsfilename,
return err;
}
+/*
+ * Note: fill_fs_info() uses functions such as make_mdn() that modify
+ * the input dnode_end_t parameter. However, we should not allow it.
+ * Therefore, we are making mdn_in constant - fill_fs_info() makes a
+ * local copy of it.
+ */
static grub_err_t
fill_fs_info (struct grub_dirhook_info *info,
- dnode_end_t mdn, struct grub_zfs_data *data)
+ const dnode_end_t *mdn_in, struct grub_zfs_data *data)
{
grub_err_t err;
dnode_end_t dn;
grub_uint64_t objnum;
grub_uint64_t headobj;
+ dnode_end_t mdn;
+
+ grub_memcpy (&mdn, mdn_in, sizeof (*mdn_in));
grub_memset (info, 0, sizeof (*info));
@@ -4148,7 +4157,7 @@ iterate_zap_fs (const char *name, grub_uint64_t val,
if (mdn.dn.dn_type != DMU_OT_DSL_DIR)
return 0;
- err = fill_fs_info (&info, mdn, ctx->data);
+ err = fill_fs_info (&info, &mdn, ctx->data);
if (err)
{
grub_errno = 0;
@@ -4179,7 +4188,7 @@ iterate_zap_snap (const char *name, grub_uint64_t val,
if (mdn.dn.dn_type != DMU_OT_DSL_DATASET)
return 0;
- err = fill_fs_info (&info, mdn, ctx->data);
+ err = fill_fs_info (&info, &mdn, ctx->data);
if (err)
{
grub_errno = 0;
@@ -4224,7 +4233,7 @@ grub_zfs_dir (grub_device_t device, const char *path,
dnode_end_t dn;
struct grub_dirhook_info info;
- err = fill_fs_info (&info, data->dnode, data);
+ err = fill_fs_info (&info, &data->dnode, data);
if (err)
{
zfs_unmount (data);
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] fill_fs_info: pass pointer to dnode_end_t instead of value
2022-08-19 14:57 [PATCH] fill_fs_info: pass pointer to dnode_end_t instead of value Jagannathan Raman
@ 2022-08-19 15:09 ` Darren Kenny
2022-08-19 15:11 ` Jag Raman
0 siblings, 1 reply; 4+ messages in thread
From: Darren Kenny @ 2022-08-19 15:09 UTC (permalink / raw)
To: Jagannathan Raman, grub-devel; +Cc: daniel.kiper
Hi Jag,
These changes look good to me.
Just to confirm, you have run 'make check' and this has no negative
impact on the zfs tests?
Assuming that is the case...
On Friday, 2022-08-19 at 10:57:22 -04, Jagannathan Raman wrote:
> Coverity reports that dnode_end_t argument of fill_fs_info() is too
> large to pass-by-value. Therefore, replace the argument with a pointer.
>
> Fixes: CID 73631
>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Thanks,
Darren.
> ---
> grub-core/fs/zfs/zfs.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
> index ffa0e5863..975c67242 100644
> --- a/grub-core/fs/zfs/zfs.c
> +++ b/grub-core/fs/zfs/zfs.c
> @@ -3983,14 +3983,23 @@ grub_zfs_getmdnobj (grub_device_t dev, const char *fsfilename,
> return err;
> }
>
> +/*
> + * Note: fill_fs_info() uses functions such as make_mdn() that modify
> + * the input dnode_end_t parameter. However, we should not allow it.
> + * Therefore, we are making mdn_in constant - fill_fs_info() makes a
> + * local copy of it.
> + */
> static grub_err_t
> fill_fs_info (struct grub_dirhook_info *info,
> - dnode_end_t mdn, struct grub_zfs_data *data)
> + const dnode_end_t *mdn_in, struct grub_zfs_data *data)
> {
> grub_err_t err;
> dnode_end_t dn;
> grub_uint64_t objnum;
> grub_uint64_t headobj;
> + dnode_end_t mdn;
> +
> + grub_memcpy (&mdn, mdn_in, sizeof (*mdn_in));
>
> grub_memset (info, 0, sizeof (*info));
>
> @@ -4148,7 +4157,7 @@ iterate_zap_fs (const char *name, grub_uint64_t val,
> if (mdn.dn.dn_type != DMU_OT_DSL_DIR)
> return 0;
>
> - err = fill_fs_info (&info, mdn, ctx->data);
> + err = fill_fs_info (&info, &mdn, ctx->data);
> if (err)
> {
> grub_errno = 0;
> @@ -4179,7 +4188,7 @@ iterate_zap_snap (const char *name, grub_uint64_t val,
> if (mdn.dn.dn_type != DMU_OT_DSL_DATASET)
> return 0;
>
> - err = fill_fs_info (&info, mdn, ctx->data);
> + err = fill_fs_info (&info, &mdn, ctx->data);
> if (err)
> {
> grub_errno = 0;
> @@ -4224,7 +4233,7 @@ grub_zfs_dir (grub_device_t device, const char *path,
> dnode_end_t dn;
> struct grub_dirhook_info info;
>
> - err = fill_fs_info (&info, data->dnode, data);
> + err = fill_fs_info (&info, &data->dnode, data);
> if (err)
> {
> zfs_unmount (data);
> --
> 2.20.1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] fill_fs_info: pass pointer to dnode_end_t instead of value
2022-08-19 15:09 ` Darren Kenny
@ 2022-08-19 15:11 ` Jag Raman
2022-08-19 16:42 ` Daniel Kiper
0 siblings, 1 reply; 4+ messages in thread
From: Jag Raman @ 2022-08-19 15:11 UTC (permalink / raw)
To: Darren Kenny; +Cc: grub-devel@gnu.org, Daniel Kiper
> On Aug 19, 2022, at 11:09 AM, Darren Kenny <darren.kenny@oracle.com> wrote:
>
> Hi Jag,
>
> These changes look good to me.
>
> Just to confirm, you have run 'make check' and this has no negative
> impact on the zfs tests?
Hi Darren,
Yes, I ran ‘make check’ and this patch had no negative impact on the ZFS test.
>
> Assuming that is the case...
>
> On Friday, 2022-08-19 at 10:57:22 -04, Jagannathan Raman wrote:
>> Coverity reports that dnode_end_t argument of fill_fs_info() is too
>> large to pass-by-value. Therefore, replace the argument with a pointer.
>>
>> Fixes: CID 73631
>>
>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>>
>
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Thank you!
--
Jag
>
> Thanks,
>
> Darren.
>
>> ---
>> grub-core/fs/zfs/zfs.c | 17 +++++++++++++----
>> 1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
>> index ffa0e5863..975c67242 100644
>> --- a/grub-core/fs/zfs/zfs.c
>> +++ b/grub-core/fs/zfs/zfs.c
>> @@ -3983,14 +3983,23 @@ grub_zfs_getmdnobj (grub_device_t dev, const char *fsfilename,
>> return err;
>> }
>>
>> +/*
>> + * Note: fill_fs_info() uses functions such as make_mdn() that modify
>> + * the input dnode_end_t parameter. However, we should not allow it.
>> + * Therefore, we are making mdn_in constant - fill_fs_info() makes a
>> + * local copy of it.
>> + */
>> static grub_err_t
>> fill_fs_info (struct grub_dirhook_info *info,
>> - dnode_end_t mdn, struct grub_zfs_data *data)
>> + const dnode_end_t *mdn_in, struct grub_zfs_data *data)
>> {
>> grub_err_t err;
>> dnode_end_t dn;
>> grub_uint64_t objnum;
>> grub_uint64_t headobj;
>> + dnode_end_t mdn;
>> +
>> + grub_memcpy (&mdn, mdn_in, sizeof (*mdn_in));
>>
>> grub_memset (info, 0, sizeof (*info));
>>
>> @@ -4148,7 +4157,7 @@ iterate_zap_fs (const char *name, grub_uint64_t val,
>> if (mdn.dn.dn_type != DMU_OT_DSL_DIR)
>> return 0;
>>
>> - err = fill_fs_info (&info, mdn, ctx->data);
>> + err = fill_fs_info (&info, &mdn, ctx->data);
>> if (err)
>> {
>> grub_errno = 0;
>> @@ -4179,7 +4188,7 @@ iterate_zap_snap (const char *name, grub_uint64_t val,
>> if (mdn.dn.dn_type != DMU_OT_DSL_DATASET)
>> return 0;
>>
>> - err = fill_fs_info (&info, mdn, ctx->data);
>> + err = fill_fs_info (&info, &mdn, ctx->data);
>> if (err)
>> {
>> grub_errno = 0;
>> @@ -4224,7 +4233,7 @@ grub_zfs_dir (grub_device_t device, const char *path,
>> dnode_end_t dn;
>> struct grub_dirhook_info info;
>>
>> - err = fill_fs_info (&info, data->dnode, data);
>> + err = fill_fs_info (&info, &data->dnode, data);
>> if (err)
>> {
>> zfs_unmount (data);
>> --
>> 2.20.1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] fill_fs_info: pass pointer to dnode_end_t instead of value
2022-08-19 15:11 ` Jag Raman
@ 2022-08-19 16:42 ` Daniel Kiper
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Kiper @ 2022-08-19 16:42 UTC (permalink / raw)
To: Jag Raman; +Cc: Darren Kenny, grub-devel@gnu.org
On Fri, Aug 19, 2022 at 05:11:39PM +0200, Jag Raman wrote:
> > On Aug 19, 2022, at 11:09 AM, Darren Kenny <darren.kenny@oracle.com> wrote:
> >
> > Hi Jag,
> >
> > These changes look good to me.
> >
> > Just to confirm, you have run 'make check' and this has no negative
> > impact on the zfs tests?
>
> Hi Darren,
>
> Yes, I ran ‘make check’ and this patch had no negative impact on the ZFS test.
>
> >
> > Assuming that is the case...
> >
> > On Friday, 2022-08-19 at 10:57:22 -04, Jagannathan Raman wrote:
> >> Coverity reports that dnode_end_t argument of fill_fs_info() is too
> >> large to pass-by-value. Therefore, replace the argument with a pointer.
> >>
> >> Fixes: CID 73631
> >>
> >> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> >>
> >
> > Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-08-19 16:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-19 14:57 [PATCH] fill_fs_info: pass pointer to dnode_end_t instead of value Jagannathan Raman
2022-08-19 15:09 ` Darren Kenny
2022-08-19 15:11 ` Jag Raman
2022-08-19 16:42 ` Daniel Kiper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.