intel / metee

Apache License 2.0
20 stars 13 forks source link

Build failure on i686 #11

Closed tatokis closed 1 month ago

tatokis commented 1 month ago

Unsure if there's any point in this software being built for 32bit systems, but thought I'd share anyway in case you're interested. Noticed this while looking at build failures on Arch Linux 32.

$ gcc --version
gcc (GCC) 14.2.1 20240805
/build/intel-metee/src/metee-4.2.0/src/linux/mei.c: In function ‘__mei_fwsts’:
/build/intel-metee/src/metee-4.2.0/src/linux/mei.c:245:52: error: conversion to ‘long int’ from ‘uint32_t’ {aka ‘unsigned int’} may change the sign of the result [-Werror=sign-conversion]
  245 |         len = pread(fd, line, FWSTS_LEN, fwsts_num * FWSTS_LEN);
      |                                                    ^
ausyskin commented 1 month ago

Interesting, can't reproduce it with gcc 11.4. Can you check that below helps?

diff --git a/src/linux/mei.c b/src/linux/mei.c index f014e97..78e12b3 100644 --- a/src/linux/mei.c +++ b/src/linux/mei.c @@ -221,7 +221,7 @@ static inline int __mei_fwsts(struct mei me, const char device, uint32_t fwsts_num, uint32_t *fwsts) {

define FWSTS_FILENAME_LEN 33

-#define FWSTS_LEN 9 +#define FWSTS_LEN 9U

define CONV_BASE 16

    char path[FWSTS_FILENAME_LEN];
    int fd;
tatokis commented 1 month ago

Doesn't change anything, unfortunately.

tomasbw commented 1 month ago

The issue is that off_t, the last parameter of pread is actually a singed int, hence on i386 in theory the multiplication of uint32 may overflow. Since we already check for fwsts_num < 6 we can safely cast it to off_t

(off_t) fwsts_num * FWSTS_LEN

I guess it won't complain on all 32 bit systems

off64_t
    This type is used similar to off_t. The difference is that 
    even on 32 bit machines, where the off_t type would have 32 bits,
    off64_t has 64 bits and so is able to address files up to 2^63 bytes
    in length. When compiling with _FILE_OFFSET_BITS == 64 this type 
    is available under the name off_t.
tatokis commented 1 month ago

Can confirm that applying that cast gets rid of the warning.

ausyskin commented 1 month ago

Fixed in 4.2.1