public inbox for cygwin-patches@cygwin.com
 help / color / mirror / Atom feed
* RE: resolver //Was: [PATCH 3/7] Debug output to show both IP and port # in native b.o., a few little cosmetic improvements for consistency
@ 2022-01-17 18:29 Lavrentiev, Anton (NIH/NLM/NCBI) [C]
  2022-01-18 10:50 ` Corinna Vinschen
  0 siblings, 1 reply; 2+ messages in thread
From: Lavrentiev, Anton (NIH/NLM/NCBI) [C] @ 2022-01-17 18:29 UTC (permalink / raw)
  To: cygwin-patches

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


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: resolver //Was: [PATCH 3/7] Debug output to show both IP and port # in native b.o., a few little cosmetic improvements for consistency
  2022-01-17 18:29 resolver //Was: [PATCH 3/7] Debug output to show both IP and port # in native b.o., a few little cosmetic improvements for consistency Lavrentiev, Anton (NIH/NLM/NCBI) [C]
@ 2022-01-18 10:50 ` Corinna Vinschen
  0 siblings, 0 replies; 2+ messages in thread
From: Corinna Vinschen @ 2022-01-18 10:50 UTC (permalink / raw)
  To: cygwin-patches

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

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2022-01-18 10:50 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-17 18:29 resolver //Was: [PATCH 3/7] Debug output to show both IP and port # in native b.o., a few little cosmetic improvements for consistency Lavrentiev, Anton (NIH/NLM/NCBI) [C]
2022-01-18 10:50 ` Corinna Vinschen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).