Re: [PATCH] ubifs: fix incorrect UBIFS_DFS_DIR_LEN macro definition

From: ZhaoLong Wang <wangzhaolong1@huawei.com>
To: Zhihao Cheng <chengzhihao1@huawei.com>, <richard@nod.at>,
	<miquel.raynal@bootlin.com>, <vigneshr@ti.com>, <ada@thorsis.com>,
	<error27@gmail.com>, <Artem.Bityutskiy@nokia.com>
Cc: <linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
	<yi.zhang@huawei.com>, <yangerkun@huawei.com>
Subject: Re: [PATCH] ubifs: fix incorrect UBIFS_DFS_DIR_LEN macro definition
Date: Mon, 25 Mar 2024 16:43:10 +0800	[thread overview]
Message-ID: <bee6e666-8616-517d-8dbd-d62d9b8a2894@huawei.com> (raw)
In-Reply-To: <f14013a4-80f1-4796-9f68-92a032dfdb13@huawei.com>

Thank you very much for your comments and suggestions.

 >> A previous attempt to fix this issue in commit be076fdf8369 ("ubifs: fix
 >> snprintf() checking") by modifying the snprintf return value check 
range is
 >> insufficient. It avoids the premature function return but does not 
address
 >> the root cause of the problem. If the buffer length is inadequate, 
snprintf
 >> will truncate the output string, resulting in incorrect directory names
 >> during filesystem debugging.
 >>
 >
 > I don't think 'snprintf' ever truncated the output string in 
dbg_debugfs_init_fs(), even before be076fdf8369 ("ubifs: fix snprintf() 
checking"). The 'UBIFS_DFS_DIR_LEN' contains trailing zero byte 
according to the comments, but actually all callers treat it as real 
string length without '\0' terminated(eg. dbg_debugfs_init_fs, 
ubifs_sysfs_register).
 > So there are no actual problems here. The only problem is that the 
comment of 'UBIFS_DFS_DIR_LEN' is not consistent with its' usage, the 
simpliest way is modifying comments. If you still want to cleanup the 
code, please remove the wrong fixing tags.

Regarding my original commit message, I realize that the statement "If 
the buffer length is inadequate, snprintf will truncate the output 
string, resulting in incorrect directory names during filesystem 
debugging." is inaccurate.

`snprintf` does indeed stop writing when it reaches the specified buffer 
size and appends a null character (`'\0'`) after the last character. 
However, since the buffer size passed to `snprintf` is sufficiently 
large, the directory names are not actually truncated in the buffer.

 > If you want to clean up code, modifying sysfs related 
code(ubifs_sysfs_register) is needed too.

That's a good suggestion, I'll go through that part of the code and make
the necessary changes for consistency.

Thanks again for your valuable feedback. I'll take all your suggestions
into consideration and adjust my patch accordingly. I'll resend the
patch once I have an updated version ready.

Best regards,
ZhaoLong Wang

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/