Skip to content

Commit

Permalink
Fix redis-check-aof incorrectly considering data in manifest format a…
Browse files Browse the repository at this point in the history
…s MP-AOF (#12958)

The check in fileIsManifest misjudged the manifest file. For example,
if resp aof contains "file", it will be considered a manifest file and
the check will fail:
```
*3
$3
set
$4
file
$4
file
```

In #12951, if the preamble aof also contains it, it will also fail.
Fixes #12951.

the bug was happening if the the word "file" is mentioned
in the first 1024 lines of the AOF. and now as soon as it finds
a non-comment line it'll break (if it contains "file" or doesn't)

Signed-off-by: Ping Xie <pingxie@google.com>
  • Loading branch information
enjoy-binbin authored and PingXie committed Jul 1, 2024
1 parent 931886c commit 4d5420e
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 2 deletions.
4 changes: 3 additions & 1 deletion src/aof.c
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,9 @@ aofInfo *aofInfoDup(aofInfo *orig) {
return ai;
}

/* Format aofInfo as a string and it will be a line in the manifest. */
/* Format aofInfo as a string and it will be a line in the manifest.
*
* When update this format, make sure to update valkey-check-aof as well. */
sds aofInfoFormat(sds buf, aofInfo *ai) {
sds filename_repr = NULL;

Expand Down
10 changes: 9 additions & 1 deletion src/valkey-check-aof.c
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ int checkSingleAof(char *aof_filename, char *aof_filepath, int last_file, int fi
struct redis_stat sb;
if (redis_fstat(fileno(fp),&sb) == -1) {
printf("Cannot stat file: %s, aborting...\n", aof_filename);
fclose(fp);
exit(1);
}

Expand Down Expand Up @@ -343,6 +344,7 @@ int fileIsRDB(char *filepath) {
struct redis_stat sb;
if (redis_fstat(fileno(fp), &sb) == -1) {
printf("Cannot stat file: %s\n", filepath);
fclose(fp);
exit(1);
}

Expand Down Expand Up @@ -379,6 +381,7 @@ int fileIsManifest(char *filepath) {
struct redis_stat sb;
if (redis_fstat(fileno(fp), &sb) == -1) {
printf("Cannot stat file: %s\n", filepath);
fclose(fp);
exit(1);
}

Expand All @@ -395,15 +398,20 @@ int fileIsManifest(char *filepath) {
break;
} else {
printf("Cannot read file: %s\n", filepath);
fclose(fp);
exit(1);
}
}

/* Skip comments lines */
/* We will skip comments lines.
* At present, the manifest format is fixed, see aofInfoFormat.
* We will break directly as long as it encounters other items. */
if (buf[0] == '#') {
continue;
} else if (!memcmp(buf, "file", strlen("file"))) {
is_manifest = 1;
} else {
break;
}
}

Expand Down
25 changes: 25 additions & 0 deletions tests/integration/aof.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,18 @@ tags {"aof external:skip"} {
assert_match "*Start checking Old-Style AOF*is valid*" $result
}

test {Test valkey-check-aof for old style resp AOF - has data in the same format as manifest} {
create_aof $aof_dirpath $aof_file {
append_to_aof [formatCommand set file file]
append_to_aof [formatCommand set "file appendonly.aof.2.base.rdb seq 2 type b" "file appendonly.aof.2.base.rdb seq 2 type b"]
}

catch {
exec src/valkey-check-aof $aof_file
} result
assert_match "*Start checking Old-Style AOF*is valid*" $result
}

test {Test valkey-check-aof for old style rdb-preamble AOF} {
catch {
exec src/valkey-check-aof tests/assets/rdb-preamble.aof
Expand Down Expand Up @@ -577,6 +589,19 @@ tags {"aof external:skip"} {
assert_match "*Start checking Multi Part AOF*Start to check BASE AOF (RDB format)*DB preamble is OK, proceeding with AOF tail*BASE AOF*is valid*Start to check INCR files*INCR AOF*is valid*All AOF files and manifest are valid*" $result
}

test {Test valkey-check-aof for Multi Part AOF contains a format error} {
create_aof_manifest $aof_dirpath $aof_manifest_file {
append_to_manifest "file appendonly.aof.1.base.aof seq 1 type b\n"
append_to_manifest "file appendonly.aof.1.incr.aof seq 1 type i\n"
append_to_manifest "!!!\n"
}

catch {
exec src/valkey-check-aof $aof_manifest_file
} result
assert_match "*Invalid AOF manifest file format*" $result
}

test {Test valkey-check-aof only truncates the last file for Multi Part AOF in fix mode} {
create_aof $aof_dirpath $aof_base_file {
append_to_aof [formatCommand set foo hello]
Expand Down

0 comments on commit 4d5420e

Please sign in to comment.