Commit 57bc9daba35dcabdacc40fd8bab0577fa7e86367
2017-07-14T10:57:49
patch_parse: implement state machine for parsing patch headers Our code parsing Git patch headers is rather lax in parsing headers of a Git-style patch. Most notably, we do not care for the exact order in which header lines appear and as such, we may parse patch files which are not really valid after all. Furthermore, the state transitions inside of the parser are not as obvious as they could be, making it harder than required to follow its logic. To improve upon this situation, this patch introduces a real state machine to parse the patches. Instead of simply parsing each line without caring for previous state and the exact ordering, we define a set of states with their allowed transitions. This makes the patch parser more strict in only allowing valid successions of header lines. As the transition table is defined inside of a single structure with the expected line, required state as well as the state that we end up in, all state transitions are immediately obvious from just having a look at this structure. This improves both maintainability and eases reasoning about the patch parser.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171 172 173 174 175 176 177
diff --git a/src/patch_parse.c b/src/patch_parse.c
index a531eec..2f0b257 100644
--- a/src/patch_parse.c
+++ b/src/patch_parse.c
@@ -372,31 +372,73 @@ static int parse_header_dissimilarity(
return 0;
}
+static int parse_header_start(git_patch_parsed *patch, git_patch_parse_ctx *ctx)
+{
+ if (parse_header_path(&patch->header_old_path, ctx) < 0)
+ return parse_err("corrupt old path in git diff header at line %"PRIuZ,
+ ctx->line_num);
+
+ if (parse_advance_ws(ctx) < 0 ||
+ parse_header_path(&patch->header_new_path, ctx) < 0)
+ return parse_err("corrupt new path in git diff header at line %"PRIuZ,
+ ctx->line_num);
+
+ return 0;
+}
+
+typedef enum {
+ STATE_START,
+
+ STATE_DIFF,
+ STATE_FILEMODE,
+ STATE_MODE,
+ STATE_INDEX,
+ STATE_PATH,
+
+ STATE_SIMILARITY,
+ STATE_RENAME,
+ STATE_COPY,
+
+ STATE_END,
+} parse_header_state;
+
typedef struct {
const char *str;
+ parse_header_state expected_state;
+ parse_header_state next_state;
int(*fn)(git_patch_parsed *, git_patch_parse_ctx *);
-} header_git_op;
-
-static const header_git_op header_git_ops[] = {
- { "diff --git ", NULL },
- { "@@ -", NULL },
- { "GIT binary patch", NULL },
- { "Binary files ", NULL },
- { "--- ", parse_header_git_oldpath },
- { "+++ ", parse_header_git_newpath },
- { "index ", parse_header_git_index },
- { "old mode ", parse_header_git_oldmode },
- { "new mode ", parse_header_git_newmode },
- { "deleted file mode ", parse_header_git_deletedfilemode },
- { "new file mode ", parse_header_git_newfilemode },
- { "rename from ", parse_header_renamefrom },
- { "rename to ", parse_header_renameto },
- { "rename old ", parse_header_renamefrom },
- { "rename new ", parse_header_renameto },
- { "copy from ", parse_header_copyfrom },
- { "copy to ", parse_header_copyto },
- { "similarity index ", parse_header_similarity },
- { "dissimilarity index ", parse_header_dissimilarity },
+} parse_header_transition;
+
+static const parse_header_transition transitions[] = {
+ /* Start */
+ { "diff --git " , STATE_START, STATE_DIFF, parse_header_start },
+
+ { "deleted file mode " , STATE_DIFF, STATE_FILEMODE, parse_header_git_deletedfilemode },
+ { "new file mode " , STATE_DIFF, STATE_FILEMODE, parse_header_git_newfilemode },
+ { "old mode " , STATE_DIFF, STATE_MODE, parse_header_git_oldmode },
+ { "new mode " , STATE_MODE, STATE_END, parse_header_git_newmode },
+
+ { "index " , STATE_FILEMODE, STATE_INDEX, parse_header_git_index },
+ { "index " , STATE_DIFF, STATE_INDEX, parse_header_git_index },
+ { "index " , STATE_END, STATE_INDEX, parse_header_git_index },
+
+ { "--- " , STATE_INDEX, STATE_PATH, parse_header_git_oldpath },
+ { "+++ " , STATE_PATH, STATE_END, parse_header_git_newpath },
+ { "GIT binary patch" , STATE_INDEX, STATE_END, NULL },
+ { "Binary files " , STATE_INDEX, STATE_END, NULL },
+
+ { "similarity index " , STATE_DIFF, STATE_SIMILARITY, parse_header_similarity },
+ { "dissimilarity index ", STATE_DIFF, STATE_SIMILARITY, parse_header_dissimilarity },
+ { "rename from " , STATE_SIMILARITY, STATE_RENAME, parse_header_renamefrom },
+ { "rename old " , STATE_SIMILARITY, STATE_RENAME, parse_header_renamefrom },
+ { "copy from " , STATE_SIMILARITY, STATE_COPY, parse_header_copyfrom },
+ { "rename to " , STATE_RENAME, STATE_END, parse_header_renameto },
+ { "rename new " , STATE_RENAME, STATE_END, parse_header_renameto },
+ { "copy to " , STATE_COPY, STATE_END, parse_header_copyto },
+
+ /* Next patch */
+ { "diff --git " , STATE_END, 0, NULL },
+ { "@@ -" , STATE_END, 0, NULL },
};
static int parse_header_git(
@@ -405,44 +447,32 @@ static int parse_header_git(
{
size_t i;
int error = 0;
-
- /* Parse the diff --git line */
- if (parse_advance_expected_str(ctx, "diff --git ") < 0)
- return parse_err("corrupt git diff header at line %"PRIuZ, ctx->line_num);
-
- if (parse_header_path(&patch->header_old_path, ctx) < 0)
- return parse_err("corrupt old path in git diff header at line %"PRIuZ,
- ctx->line_num);
-
- if (parse_advance_ws(ctx) < 0 ||
- parse_header_path(&patch->header_new_path, ctx) < 0)
- return parse_err("corrupt new path in git diff header at line %"PRIuZ,
- ctx->line_num);
+ parse_header_state state = STATE_START;
/* Parse remaining header lines */
- for (parse_advance_line(ctx);
- ctx->remain_len > 0;
- parse_advance_line(ctx)) {
-
+ for (; ctx->remain_len > 0; parse_advance_line(ctx)) {
bool found = false;
if (ctx->line_len == 0 || ctx->line[ctx->line_len - 1] != '\n')
break;
- for (i = 0; i < ARRAY_SIZE(header_git_ops); i++) {
- const header_git_op *op = &header_git_ops[i];
- size_t op_len = strlen(op->str);
+ for (i = 0; i < ARRAY_SIZE(transitions); i++) {
+ const parse_header_transition *transition = &transitions[i];
+ size_t op_len = strlen(transition->str);
- if (memcmp(ctx->line, op->str, min(op_len, ctx->line_len)) != 0)
+ if (transition->expected_state != state ||
+ memcmp(ctx->line, transition->str, min(op_len, ctx->line_len)) != 0)
continue;
+ state = transition->next_state;
+
/* Do not advance if this is the patch separator */
- if (op->fn == NULL)
+ if (transition->fn == NULL)
goto done;
parse_advance_chars(ctx, op_len);
- if ((error = op->fn(patch, ctx)) < 0)
+ if ((error = transition->fn(patch, ctx)) < 0)
goto done;
parse_advance_ws(ctx);
@@ -456,7 +486,7 @@ static int parse_header_git(
found = true;
break;
}
-
+
if (!found) {
error = parse_err("invalid patch header at line %"PRIuZ,
ctx->line_num);
@@ -464,6 +494,11 @@ static int parse_header_git(
}
}
+ if (state != STATE_END) {
+ error = parse_err("unexpected header line %"PRIuZ, ctx->line_num);
+ goto done;
+ }
+
done:
return error;
}