Skip to content

Fix settings serialization to not serialize secure settings or not take the total size into account#25323

Merged
s1monw merged 2 commits intoelastic:masterfrom
s1monw:fix_secure_setting_serialization
Jun 21, 2017
Merged

Fix settings serialization to not serialize secure settings or not take the total size into account#25323
s1monw merged 2 commits intoelastic:masterfrom
s1monw:fix_secure_setting_serialization

Conversation

@s1monw
Copy link
Copy Markdown
Contributor

@s1monw s1monw commented Jun 20, 2017

No description provided.

@s1monw s1monw added :Core/Infra/Settings Settings infrastructure and APIs >bug v5.6.0 v6.0.0 labels Jun 20, 2017
@s1monw s1monw requested review from jaymode and rjernst June 20, 2017 21:46
@s1monw s1monw merged commit 406a15e into elastic:master Jun 21, 2017
@s1monw s1monw deleted the fix_secure_setting_serialization branch June 21, 2017 06:14
s1monw added a commit that referenced this pull request Jun 21, 2017
hub-cap pushed a commit that referenced this pull request Aug 21, 2017
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Feb 21, 2018
Today, we serialize collections prefixed by their lengths. If the serialized
length is inconsistent with the number of objects in the collection then the
serialization succeeds but any subsequent deserialization will (almost
certainly) fail, reporting something unexpected at some later point in the
stream. This can happen, for instance, because of a concurrent modification
(e.g. elastic#28718) to the collection in between getting its length and iterating through
its objects.

This is a royal pain to debug, because the failure is reported a long way from
where it actually occurs. See also e.g. elastic#25323.

This change adds checks to StreamOutput to verify that it wrote as many objects
as it said it would, in order to catch any similar problems in future closer to
their sources.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants