On 6/6/2019 6:20 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> + if (stat(chain_name, &st)) {
>> ...
>> + if (st.st_size <= the_hash_algo->hexsz) {
>> ...
>> + fp = fopen(chain_name, "r");
>> + free(chain_name);
>> +
>> + if (!fp)
>> + return NULL;
>
> Checking for size before opening is an invitation for an unnecessary
> race, isn't it? Perhaps fopen() followed by fstat() is a better
> alternative?
>
>> + oids = xcalloc(st.st_size / (the_hash_algo->hexsz + 1), sizeof(struct object_id));
>> +
>> + while (strbuf_getline_lf(&line, fp) != EOF && valid) {
>> + char *graph_name;
>> + struct commit_graph *g;
>
> I am imagining an evil tester growing the file after you called
> xcalloc() above ;-) Should we at least protect ourselves not to read
> more than we planned to read originally? I would imagine that the
> ideal code organization would be more like
>
> valid = 1; have_read_all = 0;
>
> fopen();
> fstat(fp->fileno);
> count = st.st_size / hashsize;
> oids = xcalloc();
>
> for (i = 0; i < count; i++) {
> if (getline() == EOF) {
> have_read_all = 1;
> break;
> }
> add one graph based on the line;
> if (error) {
> valid = 0;
> break;
> }
> }
> if (valid && i < count)
> die("file truncated while we are reading?");
> if (valid && !have_read_all)
> die("file grew while we are reading?");
>
> if we really care, but even without going to that extreme, at least
> we should refrain from reading more than we allocated.
Thanks! I clearly was not careful enough with this input, which should
have been easy to get right. I think all your points are valid. The
code looks much cleaner after rewriting it to care about counts and to
properly order the stat() call.
-Stolee
