Re: [PATCH] fs/orangefs: use snprintf() instead of sprintf()

[Mike Marshall]

Hi Amir...

I added your patch to 6.16-rc3 and ran it through xfstests, it
works fine. snprintf won't overflow the buffer and gives you
the opportunity to compare the return code to the buffer size
to detect truncation. It doesn't look like many of the other uses
of snprintf in the kernel bother with checking for truncation.

Whatever Al was pointing out is probably important... :-)

-Mike

On Mon, Jun 23, 2025 at 1:02 PM Mike Marshall <hubcap@xxxxxxxxxxxx> wrote:
>
> Hi Y'all...
>
> I was about to add Amir's patch on top of 6.16-rc3 and run it through
> xfstests, when I saw Al's comment.
>
> Al patched a similar bit of code in orangefs-debugfs.c without
> removing sprintf:
>
> 45063097 - "don't open-code file_inode()"
>
> When I look at orangefs_debug_read as it is now, I might be trusting
> file->private_data's length too much and Amir's patch might risk sending
> a bad sprintf_ret to simple_read_from_buffer. Al, could you be
> more explicit?
>
> -Mike
>
> On Sun, Jun 22, 2025 at 4:10 PM Amir Mohammad Jahangirzad
> <a.jahangirzad@xxxxxxxxx> wrote:
> >
> > On Sun, Jun 22, 2025 at 10:18 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Sun, Jun 22, 2025 at 10:09:58PM +0330, Amir Mohammad Jahangirzad wrote:
> > >
> > > > > Replace sprintf() with snprintf() for copying the debug string
> > > > > into a temporary buffer, using ORANGEFS_MAX_DEBUG_STRING_LEN as
> > > > > the maximum size to ensure safe formatting and prevent memory
> > > > > corruption in edge cases.
> > >
> > > Out of curiosity - have you actually looked at the format used there?
> >
> > No, I just found this through static analysis. Is there any issue with it?