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

sanitize dump payload for HFE #13278

Merged
merged 10 commits into from
May 22, 2024
Merged

Conversation

sundb
Copy link
Collaborator

@sundb sundb commented May 18, 2024

Add the following validations:

  1. Get TTL using the lpGetIntegerValue() method instead of lpGetValue(), Ref Hash Field Expiration - listpack support  #13209 (comment)
  2. The TTL of listpackex is a number in the valid range (0~EB_EXPIRE_TIME_MAX) and ordered.
  3. The TTL fields of listpackex are ordered.
  4. The TTL of hashtable is within the valid range (0~EB_EXPIRE_TIME_MAX).

Other:
Fix the missing of handling OBJ_ENCODING_LISTPACK_EX in dismissHashObject().

@sundb sundb marked this pull request as ready for review May 20, 2024 10:46
@sundb sundb requested review from tezc and ronen-kalish May 20, 2024 10:47
src/rdb.c Show resolved Hide resolved
src/rdb.c Outdated Show resolved Hide resolved
if ((uint64_t)expire_at < data->last_expireat) return 0;
data->last_expireat = expire_at;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an idea: maybe lpValidateIntegrity can get an array of entry validation functions instead of a single pointer, where each such validation callback will have its own opaque data and the offset at which it should be called, from 0 to tuple_len (tuple_len is a special case for all entries, or -1 can be used for that instead).
So the validation function will be called only when reaching an entry in the listpack for which idx % tuple_len == offset, for example, if the tuple_len is 3 and the offset is 2, it will only be called for the TTL entries.
IMHO this will be a more generic approach, in which the same CB is not shared for validating different object types.

You can pass an array of such structs instead of the CB and data to lpValidateIntegrity:
typedef struct { listpackValidateEntryCB entry_cb; void *cb_userdata; int offset; } listpackEntryValidator;

And then we can have one validator for filed uniqueness and one validator for TTL validity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a good idea, however it also bring a lot of changes, and all lpValidateIntegrity* methods will be changed.
i still want to leave it as now, and optimize it in the future if it doesn't meet the needs.
i started by simply copying a new lpexValidateIntegrity, then i chose to put the TTL fields validation in _lpEntryValidation because i thought it would be the least chage.

Copy link
Collaborator

@tezc tezc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@sundb sundb changed the title Add validation for HFE to ensure data integrity sanitize dump payload for HFE May 22, 2024
@sundb sundb merged commit 95cbe87 into redis:hash-field-expiry-integ May 22, 2024
13 checks passed
@sundb sundb deleted the hfe-restore branch May 22, 2024 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants