Hi Corinna,
> Other than that, the remaining patches look good, except, adding a short
> description what patch 7 does to the commit message would be great for
> later readers of the git log.
I resubmitted the patches with a little improvement and a better description
to the #7 (now #5) as requested.
While doing the code review afresh in there, I noticed a few more problems:
1.
minires-os-if.c on line 262 does this:
262 ptr = NULL;
263 break;
and then a bit later this:
290 len = ptr - AnsPtr;
which makes the return value "len" to be a total nonsense (I think it should
return -1 in this case, but it's debatable).
2.
Also, "ptr" in the cygwin_query() function is not checked for buffer overrun
in the "done:" section of the code (after line 291), which is not good IMO.
3.
Lastly, at other places where "ptr" is checked for overrun (notably, in
write_record()),
it can leave garbage in the unfilled portion of the answer buffer (because it
still
advances "ptr" properly, to calculate the final "would-be" size of the
response):
so if the return value is greater than the passed "AnsLength", the user cannot
assume
that at least all AnsLength bytes were filled correctly. They cannot actually
assume
any "boundary" where the "Ans" buffer was actually stopped being updated.
Maybe "Ans" should be cleared upon entry?... But that would mean double-write of
the buffer in most of the use-cases (where no overflow actually occurs because
of
an adequate size of the buffer).
Anton Lavrentiev
Contractor NIH/NLM/NCBI
