After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 756260 - Applying static deltas sometimes fails
Applying static deltas sometimes fails
Status: RESOLVED FIXED
Product: ostree
Classification: Infrastructure
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: OSTree maintainer(s)
OSTree maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2015-10-08 20:46 UTC by John Hiesey
Modified: 2015-10-10 14:42 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
static-delta: Handle LZMA_BUF_ERROR returned by zlib (2.73 KB, patch)
2015-10-08 20:49 UTC, John Hiesey 		committed Details | Review
static-delta: Don't run bspatch when output object already exists (2.86 KB, patch)
2015-10-08 20:49 UTC, John Hiesey 		committed Details | Review
static-delta: Set error on bsdiff failure (1.38 KB, patch)
2015-10-08 20:49 UTC, John Hiesey 		committed Details | Review

Description John Hiesey 2015-10-08 20:46:21 UTC 
I have seen multiple different failures when applying static deltas in the latest version (2015.9) of ostree:

1. Sometimes there is an lzma error. This is due to not returning the correct special error codes from _ostree_lzma_decompressor_convert when the input buffer has length 0.

2. If some objects that the static delta generates are already present in the repo for whatever reason there are lots of failed assertions.

While debugging this I also noticed an error that isn't handled gracefully when generating static deltas: if bsdiff fails due to running out of memory, the top level g_assert (success || error) fails since error is not set properly.
Comment 1 John Hiesey 2015-10-08 20:49:24 UTC 
Created attachment 312921 [details] [review]
static-delta: Handle LZMA_BUF_ERROR returned by zlib

zlib can return LZMA_BUF_ERROR, which indicates that either
the input or output buffer has size zero. This case should cause
the correct error to be passed back from g_converter_convert
to expand the relevant buffer. Since this error is ambiguous
as to which buffer is too small, an explicit check on the
output buffer size is added as well.
Comment 2 John Hiesey 2015-10-08 20:49:29 UTC 
Created attachment 312922 [details] [review]
static-delta: Don't run bspatch when output object already exists

There is already a check that the destination object does not
exist in all other cases when processing an incoming static delta.
However, the bspatch case would still try to run and fail. Add
an analogous check to that case as well.
Comment 3 John Hiesey 2015-10-08 20:49:34 UTC 
Created attachment 312923 [details] [review]
static-delta: Set error on bsdiff failure

bsdiff can fail when generating static deltas, particularly if
not enough memory is available. Set error properly when this happens.
Comment 4 Colin Walters 2015-10-09 20:14:35 UTC 
Review of attachment 312921 [details] [review]:

Looks similar to what we're doing with the zlib compressor.  OK.
Comment 5 Colin Walters 2015-10-10 14:29:21 UTC 
Comment on attachment 312921 [details] [review]
static-delta: Handle LZMA_BUF_ERROR returned by zlib

https://git.gnome.org/browse/ostree/commit/?id=dd35e1b9cdb4b8abf0ffd15263ae33db3e729d75
Comment 6 Colin Walters 2015-10-10 14:31:22 UTC 
Review of attachment 312922 [details] [review]:

LGTM
Comment 8 Colin Walters 2015-10-10 14:37:09 UTC 
Review of attachment 312923 [details] [review]:

LGTM.  Aside, would be interesting to see if a a static analyzer could detect this.