pack: Improve error handling for get_delta_base() #5425
Conversation
|
note that the build issues are caused by mbedtls not being able to be downloaded, and are unrelated to this change: https://dev.azure.com/libgit2/libgit2/_build/results?buildId=3441&view=logs&j=bfcc9059-3690-5047-368c-49dfe7d9b8d3&t=8167bc0c-7141-585d-94fc-b6d47d2e4c77&l=1170 |
|
Seems like the certificate for https://tls.mbed.org has expired or is somehow invalid now. Let's wait a bit and if the problem persists we could just add "-k" to curl for now. @ethomson: we started caching Docker images, but we still build them on every run, right? Is this expected or are we somehow invalidating caches? Anyway, back the proposed changes, they do look like a welcome improvement to me. Does this fix a bug you've experienced? Otherwise I'd like to defer this until after v1.0 due to the current code freeze. |
|
yes, this fixes a crash that can happen if the .pack files are somehow mangled. with this change, it nicely errors out instead of crashing. the good thing is that the only garbage values that can come out of this are numbers in the range [-10, 0], so i couldn't think of ways in which this could escalate into arbitrary code execution or attacker-controlled reads. |
lhchavez
force-pushed
the
fix-get-delta-base
branch
3 times, most recently
from
1d82d7d
to
4d68ded
Compare
|
tests now pass \o/ |
We cache some of the layers; our containers are too big to cache them all. So we get some speedup here but not as much as if we could cache the entire thing. |
src/pack.c
Outdated
| if (base_offset < 0) { | ||
| error = (int)base_offset; | ||
| return error; | ||
| } |
There was a problem hiding this comment.
These are a bit awkward and don't really fit into our greater error handling patterns. Would you mind updating get_delta_base to return the base offset via an out parameter and have the direct return value indicate errors, only? Let me know if that exceeds the time you want to invest here
There was a problem hiding this comment.
I would not mind! I also found these a bit awkward and I was aiming for the change with the smallest possible delta, so I thought that a refactor in the same change was not ideal.
But if that's a possibility, I would also prefer to do the cleanup now.
Done!
This change moves the responsibility of setting the error upon failures of get_delta_base() to get_delta_base() instead of its callers. That way, the caller chan always check if the return value is negative and mark the whole operation as an error instead of using garbage values, which can lead to crashes if the .pack files are malformed.
This makes get_delta_base() return the error code as the return value and the delta base as an out-parameter.
lhchavez
force-pushed
the
fix-get-delta-base
branch
from
4d68ded
to
ba59a4a
Compare
|
I would not mind! I also found these a bit awkward and I was aiming
for the change with the smallest possible delta, so I thought that a
refactor in the same change was not ideal.
Now that we've got v1.0.0 released we can be less strict about that
again.
But if that's a possibility, I would also prefer to do the cleanup
now.
Done!
Cool, thanks a lot. I'll review your changes soonish.
|
| if (base_offset == 0) { | ||
| error = packfile_error("delta offset is zero"); | ||
| goto on_error; | ||
| } |
There was a problem hiding this comment.
You're removing this error case here, but as far as I can see it's not been added to get_delta_base.
There was a problem hiding this comment.
Done. Not sure if it's needed in both places that I added. I would assume git_oidmap_get would not store an invalid entry, so maybe I am being paranoid.
There was a problem hiding this comment.
Better be defensive while programming than running into errors because of taking too many things for granted 👍
This change moves the responsibility of setting the error upon failures
of get_delta_base() to get_delta_base() instead of its callers. That
way, the caller chan always check if the return value is negative and
mark the whole operation as an error instead of using garbage values,
which can lead to crashes if the
.packfiles are malformed.