On Fri, Nov 18, 2016 at 02:38:41AM -0700, Andreas Dilger wrote:
> > @@ -3321,17 +3321,17 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> > int err = 0;
> > unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> > ext4_group_t first_not_zeroed;
> > + char *s_mount_opts = kzalloc(sizeof(sbi->s_es->s_mount_opts)+1,
> > + GFP_KERNEL);
>
> This doesn't check if "sbi" is non-NULL before dereferencing it. While
> there isn't really any likelihood for this allocation to fail, I don't
> think there is any benefit to moving these allocations to the declaration.
This should be safe because sizeof(sbi->s_es->s_mount_opts) is a
constant, so it would be calculated at compile time. You're right
that we can use kstrndup() below, so I'll do that, though, in which
case s_mount_opts can be initialized to NULL.
> > + if (sbi->s_es->s_mount_opts[0]) {
> > + strncpy(s_mount_opts, sbi->s_es->s_mount_opts,
> > + sizeof(sbi->s_es->s_mount_opts));
>
> This could use kstrndup()? It always allocates an extra byte for the NUL.
Will do.
> > @@ -4162,7 +4166,9 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> >
> > if (___ratelimit(&ext4_mount_msg_ratelimit, "EXT4-fs mount"))
> > ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. "
> > - "Opts: %s%s%s", descr, sbi->s_es->s_mount_opts,
> > + "Opts: %.*s%s%s", descr,
> > + sizeof(sbi->s_es->s_mount_opts),
> > + sbi->s_es->s_mount_opts,
>
> This isn't really needed, since there is always a NUL terminator added
> to the string?
This is sbi->s_es->s_mount_opts, not mount_opts. And the former is
not necessarily guaranteed to be NUL terminated. And the reason why
we can't use mount_opts is because parse_options() destroys the
passed-in string as part of the parsing.
Cheers,
- Ted
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html