From: Dan Carpenter <dan.carpenter@oracle.com>
To: Hans de Goede <hdegoede@redhat.com>,
Luc Van Oostenryck <luc.vanoostenryck@gmail.com>,
linux-sparse@vger.kernel.org
Cc: Arnd Bergmann <arnd@kernel.org>,
Nathan Chancellor <nathan@kernel.org>,
Nick Desaulniers <ndesaulniers@google.com>,
Al Viro <viro@zeniv.linux.org.uk>, Arnd Bergmann <arnd@arndb.de>,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
llvm@lists.linux.dev
Subject: Re: [PATCH] vboxsf: fix old signature detection
Date: Mon, 27 Sep 2021 16:02:53 +0300 [thread overview]
Message-ID: <20210927130253.GH2083@kadam> (raw)
In-Reply-To: <40217483-1b8d-28ec-bbfc-8f979773b166@redhat.com>
GCC handles it the same way as Clang. '\377' is -1 but in Sparse it's
255. I've added the Sparse mailing list to the CC.
regards,
dan carpenter
On Mon, Sep 27, 2021 at 12:09:01PM +0200, Hans de Goede wrote:
> 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 <hdegoede@redhat.com>
>
> 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
>
>
>
>
>
>
>
>
>
>
> > 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 <dan.carpenter@oracle.com>
> > 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);
> >
next prev parent reply other threads:[~2021-09-27 13:03 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-27 9:40 [PATCH] vboxsf: fix old signature detection Arnd Bergmann
2021-09-27 10:09 ` Hans de Goede
2021-09-27 13:02 ` Dan Carpenter [this message]
2021-09-27 13:21 ` Arnd Bergmann
2021-09-27 18:33 ` Linus Torvalds
2021-09-28 9:39 ` Hans de Goede
2021-09-28 10:11 ` Arnd Bergmann
2021-09-28 10:31 ` Hans de Goede
2021-09-28 10:40 ` Arnd Bergmann
2021-09-28 9:56 ` Rasmus Villemoes
2022-05-22 19:08 ` [PATCH] fix zero/sign extension of integer character constants Luc Van Oostenryck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210927130253.GH2083@kadam \
--to=dan.carpenter@oracle.com \
--cc=arnd@arndb.de \
--cc=arnd@kernel.org \
--cc=hdegoede@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sparse@vger.kernel.org \
--cc=llvm@lists.linux.dev \
--cc=luc.vanoostenryck@gmail.com \
--cc=nathan@kernel.org \
--cc=ndesaulniers@google.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.