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

Replace sleep()/usleep() with std::chrono calls #473

Merged
merged 7 commits into from Sep 2, 2020

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Mar 21, 2020

This PR eliminates all remaining calls to sleep() and usleep() in the library (except in the Decklink files; I don't mess with those). In both C and C++, sleep() and usleep() are considered obsolete, and will be flagged as a potential security issue by many static analysis tools, due to their poor interaction with things like SIGALRM.

In C++11, the std::this_thread::sleep_for() STL class provides a feature-rich replacement.

(This is only a stopgap measure, as it would be better to eliminate these sleeps entirely. Sleeping is generally inadvisable — it's better to use timers or waitables. Here's Jules, the JUCE developer railing against the use of sleep() to delay program execution.)

@ferdnyc ferdnyc added the code Source code cleanup, streamlining, or style tweaks label Mar 21, 2020
@codecov-io
Copy link

codecov-io commented Mar 21, 2020

Codecov Report

Merging #473 into develop will increase coverage by 0.43%.
The diff coverage is 4.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #473      +/-   ##
===========================================
+ Coverage    48.33%   48.76%   +0.43%     
===========================================
  Files          128      129       +1     
  Lines         9962    10037      +75     
===========================================
+ Hits          4815     4895      +80     
+ Misses        5147     5142       -5     
Impacted Files Coverage Δ
src/Frame.cpp 47.70% <0.00%> (+1.16%) ⬆️
src/Qt/AudioPlaybackThread.cpp 0.00% <0.00%> (ø)
src/Qt/PlayerPrivate.cpp 0.00% <0.00%> (ø)
src/Qt/VideoCacheThread.cpp 0.00% <0.00%> (ø)
src/FFmpegReader.cpp 68.79% <50.00%> (ø)
src/ZmqLogger.cpp 29.33% <100.00%> (ø)
src/FFmpegWriter.cpp 62.48% <0.00%> (-0.60%) ⬇️
include/DummyReader.h 0.00% <0.00%> (ø)
tests/DummyReader_Tests.cpp 100.00% <0.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45b68ce...ef4dd6c. Read the comment docs.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Sep 2, 2020

I'm going to go ahead and merge this. It's been sitting here a while, and these usleep() calls are the only remaining security-related vulnerability still flagged in our Codacy report. Be nice to get an all-clear there.

@ferdnyc ferdnyc merged commit e500cae into OpenShot:develop Sep 2, 2020
@ferdnyc ferdnyc deleted the no-sleep-til branch September 2, 2020 06:07
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Sep 2, 2020

ARRGGH! Of course, now we still have 2 security flags left, because there are still those two usleep() calls in the DeckLink code I didn't want to touch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Source code cleanup, streamlining, or style tweaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants