Hi Anton, On Jan 17 18:29, Lavrentiev, Anton (NIH/NLM/NCBI) [C] via Cygwin-patches wrote: > 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.
Thanks! > 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). I don't think it's actually debatable. The statp->os_query call should return the same kind of reply as the calling res_nsend/res_nquery. The return values of this function calls are used unfiltered. Therefore the above snippet needs fixing and, along the same lines, it should set statp->res_h_errno to a useful value. > 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. ACK > 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). Either that or fill the reminder of the buffer with 0-bytes. Clearing upon entry is a simpe and good choice, IMHO. Yeah, it might take a tiny little bit longer, but it's a safe choice. Thanks for looking into this! Corinna
