Do not divide by zero when a download reports no Content-Length - #4551
Open
Daksha1611 wants to merge 1 commit into
Open
Do not divide by zero when a download reports no Content-Length#4551Daksha1611 wants to merge 1 commit into
Daksha1611 wants to merge 1 commit into
Conversation
print_progress() computed (float)count / max with max taken straight from libcurl's dltotal. A response using chunked transfer encoding reports dltotal == 0, and the guard in progress_callback only short-circuits while dltotal == dlnow, so as soon as any bytes arrive the callback falls through to print_progress(dlnow, 0, ...). The ratio is then infinite. Converting that to int is undefined; on x86-64 it yields INT_MIN, so the bar-fill loop does not run and the padding loop below it runs from INT_MIN to bar_width - roughly 2.1 billion putchar calls on every progress tick, once a second. The pull looks frozen and floods the terminal. Handle an unknown total explicitly by reporting the running byte count instead of a percentage, and move the bar arithmetic into computeProgressBarCells(), which returns 0 for an unknown total and clamps the result to [0, barWidth]. The clamp also covers a server reporting more bytes than it announced, which previously overran the bar. computeProgressBarCells() is declared in curl_downloader.hpp only so the arithmetic can be unit tested - print_progress() itself is a file-local static that writes to stdout. Happy to inline it and drop the tests if the smaller header surface is preferred. Tests: adds CurlDownloaderProgressTest covering the unknown-total case that caused the hang, normal ratio tracking, and clamping at both ends.
Daksha1611
marked this pull request as ready for review
September 12, 2026 07:09
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🛠 Summary
Fixes #4550.
print_progress()computed(float)count / maxwithmaxtaken straight from libcurl'sdltotal. A response using chunked transfer encoding reportsdltotal == 0, and the guard inprogress_callbackonly short-circuits whiledltotal == dlnow, so once any bytes arrive the callback falls through toprint_progress(dlnow, 0, ...).The ratio is then infinite, and converting that to
intis undefined — on x86-64 it yieldsINT_MIN, so the bar-fill loop does not run and the padding loop below it runs fromINT_MINtobar_width. That is roughly 2.1 billionprintf(" ")calls per tick, once a second, so the pull looks frozen and floods the terminal.Reports the running byte count when the total is unknown, and moves the bar arithmetic into
computeProgressBarCells(), which returns 0 for an unknown total and clamps to[0, barWidth]. The clamp also covers a server reporting more bytes than it announced, which previously overran the bar the other way.computeProgressBarCells()is declared in the header only so the arithmetic can be unit tested —print_progress()is a file-local static that writes to stdout, and there is no existing test surface for the pull module's console output. If you would rather keep the header minimal I am happy to make it static again and drop the tests. The tests live inpull_hf_model_test.cpp, whose target already linkscurl_downloadertransitively, so no BUILD change was needed.I have no OVMS build container available, so this is not compiled against the full tree and CI will need to confirm the build. I did check the helper in isolation, including reproducing the original
INT_MINresult from the two upstream lines. Draft for that reason.🧪 Checklist