Re: [PATCH] vboxsf: fix old signature detection
From: Hans de Goede <hidden>
Date: 2021-09-27 10:09:07
Also in:
linux-fsdevel, lkml
Hi, On 9/27/21 11:40 AM, Arnd Bergmann wrote:
From: Arnd Bergmann <arnd@arndb.de>
The constant-out-of-range check in clang found an actual bug in
vboxsf, which leads to the detection of old mount signatures always
failing:
fs/vboxsf/super.c:394:21: error: result of comparison of constant -3 with expression of type 'unsigned char' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
options[3] == VBSF_MOUNT_SIGNATURE_BYTE_3) {
~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
This actually seems to be a clang bug though, or at least a weird
interpretation (and different from gcc) of the C spec.
VBSF_MOUNT_SIGNATURE_BYTE_3 is defined as:
#define VBSF_MOUNT_SIGNATURE_BYTE_3 ('\375')
The C-spec:
http://port70.net/~nsz/c/c11/n1570.html#6.4.4.4p5
Says the following:
"The octal digits that follow the backslash in an octal escape sequence are taken to be part of the construction of a single character for an integer character constant or of a single wide character for a wide character constant. The numerical value of the octal integer so formed specifies the value of the desired character or wide character."
Character constants have a type of int, so 0375
clearly fits in the range of that.
I guess the problem is that gcc sees this as
const int VBSF_MOUNT_SIGNATURE_BYTE_3 = 0375;
Where as clang sees this as:
const int VBSF_MOUNT_SIGNATURE_BYTE_3 = (char)0375;
Which is a nice subtle incompatibility between the 2 :|
With that said, the patch is fine and I have no objections
against it:
Reviewed-by: Hans de Goede <redacted>
Although maybe it is better to actually remove any
ambiguity and just replace the defines with:
static const u8 VBSF_MOUNT_SIGNATURE_BYTE_0 = 0000;
static const u8 VBSF_MOUNT_SIGNATURE_BYTE_1 = 0377;
static const u8 VBSF_MOUNT_SIGNATURE_BYTE_2 = 0376;
static const u8 VBSF_MOUNT_SIGNATURE_BYTE_3 = 0375;
?
Regards,
Hans
quoted hunk ↗ jump to hunk
fs/vboxsf/super.c:393:21: error: result of comparison of constant -2 with expression of type 'unsigned char' is always false [-Werror,-Wtautological-constant-out-of-range-compare] options[2] == VBSF_MOUNT_SIGNATURE_BYTE_2 && ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~ fs/vboxsf/super.c:392:21: error: result of comparison of constant -1 with expression of type 'unsigned char' is always false [-Werror,-Wtautological-constant-out-of-range-compare] options[1] == VBSF_MOUNT_SIGNATURE_BYTE_1 && ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~ The problem is that the pointer is of type 'unsigned char' but the constant is a 'char'. My first idea was to change the type of the pointer to 'char *', but I noticed that this was the original code and it got changed after 'smatch' complained about this. I don't know if there is a bug in smatch here, but it sounds to me that clang's warning is correct. Forcing the constants to an unsigned type should make the code behave consistently and avoid the warning on both. Fixes: 9d682ea6bcc7 ("vboxsf: Fix the check for the old binary mount-arguments struct") Cc: Dan Carpenter <redacted> Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- fs/vboxsf/super.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)diff --git a/fs/vboxsf/super.c b/fs/vboxsf/super.c index 4f5e59f06284..84e2236021de 100644 --- a/fs/vboxsf/super.c +++ b/fs/vboxsf/super.c@@ -21,10 +21,10 @@ #define VBOXSF_SUPER_MAGIC 0x786f4256 /* 'VBox' little endian */ -#define VBSF_MOUNT_SIGNATURE_BYTE_0 ('\000') -#define VBSF_MOUNT_SIGNATURE_BYTE_1 ('\377') -#define VBSF_MOUNT_SIGNATURE_BYTE_2 ('\376') -#define VBSF_MOUNT_SIGNATURE_BYTE_3 ('\375') +#define VBSF_MOUNT_SIGNATURE_BYTE_0 (u8)('\000') +#define VBSF_MOUNT_SIGNATURE_BYTE_1 (u8)('\377') +#define VBSF_MOUNT_SIGNATURE_BYTE_2 (u8)('\376') +#define VBSF_MOUNT_SIGNATURE_BYTE_3 (u8)('\375') static int follow_symlinks; module_param(follow_symlinks, int, 0444);