From: Linke Li <[email protected]>
Fix undefined behavior in the code by properly handling the left shift operaion.
Instead of left-shifting a negative value, explicitly cast -1 to an unsigned int
before the shift. This ensures well defined behavior and resolves any potential
issues.
Signed-off-by: Linke Li <[email protected]>
---
fs/isofs/util.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/isofs/util.c b/fs/isofs/util.c
index e88dba721661..4c902901401a 100644
--- a/fs/isofs/util.c
+++ b/fs/isofs/util.c
@@ -37,7 +37,7 @@ int iso_date(u8 *p, int flag)
/* sign extend */
if (tz & 0x80)
- tz |= (-1 << 8);
+ tz |= ((unsigned int)-1 << 8);
/*
* The timezone offset is unreliable on some disks,
--
2.25.1
On Sun, Jul 09, 2023 at 02:42:55PM +0800, Linke Li wrote:
> From: Linke Li <[email protected]>
>
> Fix undefined behavior in the code by properly handling the left shift operaion.
> Instead of left-shifting a negative value, explicitly cast -1 to an unsigned int
> before the shift. This ensures well defined behavior and resolves any potential
> issues.
This certainly fixes the problem, but wouldn't it be easier to get the
compiler to do the work for us?
#include <stdio.h>
int f(unsigned char *p)
{
return (signed char)p[0];
}
int main(void)
{
unsigned char x = 0xa5;
printf("%d\n", f(&x));
return 0;
}
prints -91.
Dear Markus,
Thank you for your valuable feedback, I apologize for my typo in the
description.
> How do you think about to add the tag “Fixes”?
I agree with that.
It looks like maybe there is an issue with "year" as well.
fs/isofs/util.c
19 int iso_date(u8 *p, int flag)
20 {
21 int year, month, day, hour, minute, second, tz;
22 int crtime;
23
24 year = p[0];
^^^^^
year is 0-255.
25 month = p[1];
26 day = p[2];
27 hour = p[3];
28 minute = p[4];
29 second = p[5];
30 if (flag == 0) tz = p[6]; /* High sierra has no time zone */
31 else tz = 0;
32
33 if (year < 0) {
^^^^^^^^
But this checks year for < 0 which is impossible. Should it be:
year = (signed char)p[0];?
34 crtime = 0;
35 } else {
36 crtime = mktime64(year+1900, month, day, hour, minute, second);
37
38 /* sign extend */
39 if (tz & 0x80)
40 tz |= (-1 << 8);
41
42 /*
regards,
dan carpenter
Thanks for your reply.
> This certainly fixes the problem, but wouldn't it be easier to get the
> compiler to do the work for us?
I don't know which solution is better, but it does avoid this problem.
Like
tz = (int)(signed char)p[6];
From: Dan Carpenter
> Sent: 10 July 2023 10:57
>
> It looks like maybe there is an issue with "year" as well.
>
> fs/isofs/util.c
> 19 int iso_date(u8 *p, int flag)
> 20 {
> 21 int year, month, day, hour, minute, second, tz;
> 22 int crtime;
> 23
> 24 year = p[0];
> ^^^^^
> year is 0-255.
....
> 32
> 33 if (year < 0) {
> ^^^^^^^^
> But this checks year for < 0 which is impossible. Should it be:
>
> year = (signed char)p[0];?
Or not?
What happens in 2027 ?
I bet the value has to be treated an unsigned.
>
> 34 crtime = 0;
> 35 } else {
> 36 crtime = mktime64(year+1900, month, day, hour, minute, second);
> 37
> 38 /* sign extend */
> 39 if (tz & 0x80)
> 40 tz |= (-1 << 8);
Just change the definition of tz from 'int' to 's8'
and it will all happen 'by magic'.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Thu, Jul 13, 2023 at 02:11:02PM +0000, David Laight wrote:
> From: Dan Carpenter
> > Sent: 10 July 2023 10:57
> >
> > It looks like maybe there is an issue with "year" as well.
> >
> > fs/isofs/util.c
> > 19 int iso_date(u8 *p, int flag)
> > 20 {
> > 21 int year, month, day, hour, minute, second, tz;
> > 22 int crtime;
> > 23
> > 24 year = p[0];
> > ^^^^^
> > year is 0-255.
> ....
> > 32
> > 33 if (year < 0) {
> > ^^^^^^^^
> > But this checks year for < 0 which is impossible. Should it be:
> >
> > year = (signed char)p[0];?
>
> Or not?
>
> What happens in 2027 ?
> I bet the value has to be treated an unsigned.
Yeah. Good point. We could delete that if statement and pull the whole
function in a tab.
regards,
dan carpenter