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 large timestamp causing overflow #3106

Merged
merged 2 commits into from
Dec 23, 2023

Conversation

yagehu
Copy link
Contributor

@yagehu yagehu commented Dec 21, 2023

This commit fixes a bug in toTimespec. The duration overflows with a large timestamp because std::chrono::nanoseconds is signed 64-bit integer. Instead, we should explicitly use uint64_t because __wasi_timestamp_t is unsigned 64-bit.

fixes #3105

Copy link
Member

juntao commented Dec 21, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Commit dbb3ae3af505a3e4825d5f13db9b79ec22d25f73

The key change made in this patch is in the function toTimespec within the file lib/host/wasi/linux.h. Yage Hu has replaced the type of the Total variable from std::chrono::nanoseconds to std::chrono::duration<uint64_t, std::nano> to try to prevent an overflow which occurs when the input Timestamp in the '__wasi_timestamp_t' format is too large.

However, potential issues exist. The conversion between uint64_t and time duration might not behave as expected if the underlying library or platform doesn't correctly support uint64_t or if negative timestamps are ever allowed.

Another point to consider is whether the rest of the code and interacting systems can safely handle uint64_t values, as this change might produce unexpected results elsewhere in the system.

Finally, one would need to verify if overflow is the only issue with large timestamps. Other potential problems like precision loss or rounding errors should also be checked. Depending on how this function is used, such problems might need to be handled as well.

All these considerations should be tested as part of a comprehensive set of unit and integration tests to ensure the functionality remains intact after this change.

This commit fixes a bug in `toTimespec`.  The duration overflows with a
large timestamp because `std::chrono::nanoseconds` is signed 64-bit
integer.  Instead, we should explicitly use `uint64_t` because
`__wasi_timestamp_t` is unsigned 64-bit.

Signed-off-by: Yage Hu <me@huyage.dev>
Copy link

codecov bot commented Dec 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ab994af) 80.84% compared to head (7edfb5e) 80.84%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3106   +/-   ##
=======================================
  Coverage   80.84%   80.84%           
=======================================
  Files         159      159           
  Lines       23035    23035           
  Branches     4734     4734           
=======================================
+ Hits        18622    18623    +1     
  Misses       3131     3131           
+ Partials     1282     1281    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alabulei1
Copy link
Contributor

@hydai Can you please take a look? Thanks.

@hydai
Copy link
Member

hydai commented Dec 22, 2023

Hi @yagehu
I found some WASI tests failed. Could you check if this is related to your commit or not?

@yagehu
Copy link
Contributor Author

yagehu commented Dec 22, 2023

@hydai The failing wasi-testsuite case is pwrite-with-append, which is a newly added case related to #3062. So it is not related. Maybe I should remove this test case, it seems most runtimes are failing this and it is not trivial to fix and there's no consensus.

@hydai hydai merged commit 0f9153f into WasmEdge:master Dec 23, 2023
35 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fd_filestat_set_times with a large timestamp leads to errno inval
4 participants