Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sys_stat size integer overflow #1157

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

barisyild
Copy link

This pull request fixes the file size integer overflow issue for sys_stat.

@barisyild barisyild changed the title Fix sys stat size integer overflow Fix sys_stat size integer overflow Oct 14, 2024
@Simn
Copy link
Member

Simn commented Oct 14, 2024

CI failure is related:

Error: /Users/runner/work/hxcpp/hxcpp/src/hx/libs/std/Sys.cpp:479:4: error: conversion from 'long' to 'const Dynamic' is ambiguous
   STATF64(size);

@barisyild
Copy link
Author

CI failure is related:

Error: /Users/runner/work/hxcpp/hxcpp/src/hx/libs/std/Sys.cpp:479:4: error: conversion from 'long' to 'const Dynamic' is ambiguous
   STATF64(size);

I think Apple is a bit more strict with the type casting, the compile problem was solved.

https://github.com/barisyild/hxcpp/actions/runs/11325731311

@Apprentice-Alchemist
Copy link
Contributor

Note that long is 32-bit on certain platforms, it would be better to use int64_t.

@Simn
Copy link
Member

Simn commented Oct 14, 2024

Can't we just always use the __APPLE__ version?

@barisyild
Copy link
Author

Obviously nothing has changed on the apple side, but the error occurred, does anyone have any idea?

Old Commit:
a427d1f

New Commit:
193b3d1

@barisyild
Copy link
Author

Probably a bug, because the same code works fine in my ci.

https://github.com/barisyild/hxcpp/actions/runs/11331298885

@Aidan63
Copy link
Contributor

Aidan63 commented Oct 15, 2024

Doesn't this require a change on the haxe side to work properly? Since the FileStat typedef only has size as an Int.

Changes like this should also probably be guarded with a HXCPP_API_LEVEL check so new hxcpp versions can be used with older haxe without anything changing.

@barisyild
Copy link
Author

barisyild commented Oct 15, 2024

Doesn't this require a change on the haxe side to work properly? Since the FileStat typedef only has size as an Int.

Changes like this should also probably be guarded with a HXCPP_API_LEVEL check so new hxcpp versions can be used with older haxe without anything changing.

Even if there is no api change on the Haxe side, it looks fine, because haxe int actually works with float double-precision.

https://github.com/HaxeFoundation/haxe/blob/94bde7d3c00b0d1d5eabf0bbde93533c4a44b065/std/StdTypes.hx#L61

@barisyild
Copy link
Author

Doesn't this require a change on the haxe side to work properly? Since the FileStat typedef only has size as an Int.

Changes like this should also probably be guarded with a HXCPP_API_LEVEL check so new hxcpp versions can be used with older haxe without anything changing.

I tried and you are right, I think it needs a haxe api change.

@barisyild
Copy link
Author

@skial skial mentioned this pull request Oct 31, 2024
1 task
@SomeGuyWhoLovesCoding
Copy link

I just tried it and it doesn't work. FileStat.size just returns 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants