Re: git: aae67a2c2b66 - main - mfiutil: Fix unsafe assumptions of snprintf(3) return value

On Sat, Jun 14, 2025 at 6:42 AM John Baldwin <jhb@freebsd.org> wrote:
>
> On 6/12/25 21:21, Warner Losh wrote:
> > The branch main has been updated by imp:
> >
> > URL: https://cgit.FreeBSD.org/src/commit/?id=aae67a2c2b663a6bce8fbc087ff8490336b8618f
> >
> > commit aae67a2c2b663a6bce8fbc087ff8490336b8618f
> > Author:     WHR <whr@rivoreo.one>
> > AuthorDate: 2024-09-03 10:19:04 +0000
> > Commit:     Warner Losh <imp@FreeBSD.org>
> > CommitDate: 2025-06-13 01:21:44 +0000
> >
> >      mfiutil: Fix unsafe assumptions of snprintf(3) return value
> >
> >      PR: 281160
> >      Reviewed by: imp
> >      Pull Request: https://github.com/freebsd/freebsd-src/pull/1405
> >      Closes: https://github.com/freebsd/freebsd-src/pull/1405
> > ---
> >   usr.sbin/mfiutil/mfi_bbu.c | 17 +++++++++++++++--
> >   1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/usr.sbin/mfiutil/mfi_bbu.c b/usr.sbin/mfiutil/mfi_bbu.c
> > index 9075c4d0ddd0..e97227d47c70 100644
> > --- a/usr.sbin/mfiutil/mfi_bbu.c
> > +++ b/usr.sbin/mfiutil/mfi_bbu.c
> > @@ -50,10 +50,23 @@ mfi_autolearn_period(uint32_t period, char *buf, size_t sz)
> >
> >       tmp = buf;
> >       if (d != 0) {
> > -             tmp += snprintf(buf, sz, "%u day%s", d, d == 1 ? "" : "s");
> > +             int fmt_len;
> > +             fmt_len = snprintf(buf, sz, "%u day%s", d, d == 1 ? "" : "s");
> > +             if (fmt_len < 0) {
> > +                     *buf = 0;
> > +                     return;
> > +             }
> > +             if ((size_t)fmt_len >= sz) {
> > +                     return;
> > +             }
> > +             tmp += fmt_len;
> >               sz -= tmp - buf;
> >               if (h != 0) {
> > -                     tmp += snprintf(tmp, sz, ", ");
> > +                     fmt_len = snprintf(tmp, sz, ", ");
> > +                     if (fmt_len < 0 || (size_t)fmt_len >= sz) {
> > +                             return;
> > +                     }
> > +                     tmp += fmt_len;
> >                       sz -= 2;
> >               }
> >       }
>
> It seems like using a string builder like fmemopen() or sbuf() would be
> better here than fragile dances with snprintf().

True. This is better than what was there, but either of those would be better.

Warner