Re: [PATCH gwproxy v10 2/2] gwproxy: refactor code base to add experimental raw DNS backend

[Ahmad Gani <reyuki@gnuweeb.org>]

On Thu, Sep 11, 2025 at 5:11 PM Alviro Iskandar Setiawan wrote:
> On Wed, Sep 10, 2025 at 5:44 PM Ahmad Gani wrote:
> > +       cfg = &w->ctx->cfg;
> > +       resolv = &w->dns_resolver;
> > +       p1 = realloc(resolv->stack.arr, cfg->sess_map_cap * sizeof(*resolv->stack.arr));
> > +       if (!p1)
> > +               return;
> > +       resolv->stack.arr = p1;
> > +
> > +       p2 = realloc(resolv->sess_map, cfg->sess_map_cap * sizeof(*resolv->sess_map));
> > +       if (!p2)
> > +               return;
> > +       resolv->sess_map = p2;
>
> This shrink operation looks very *dangerous* to me. Think, what
> happens if "p2 = realloc()" fails?
>
> You update "->stack.arr = p1;" but given "->stack.top >=
> ->sess_map_cap", now ->stack.top is pointing to an index beyond the
> array capacity because you have just successfully shrunk the array
> capacity down to ->sess_map_cap. Next time, a client with SOCKS5
> hostname comes in, you call pop_txid(), and you will explode
>    ->stack.arr[--resolv->stack.top] ** OVERFLOW **
> because ->stack.top is not updated. That kind of pattern is only safe
> for array expansion, not for this shrink op definitely. You're allowed
> to over allocate the size, but not under allocate.

That does sound dangerous. Even when shrinking, realloc can still fail if
it can't allocate a smaller block.

> > +       memset(p2, 0, cfg->sess_map_cap * sizeof(*resolv->sess_map));
> > +
> > +       i = cfg->sess_map_cap;
> > +       resolv->stack.top = i--;
> > +       for (; i <= 0; i--)
> > +               p1[i] = i;
> > +
> > +       resolv->sess_map_cap = cfg->sess_map_cap;
>
> Meanwhile, ->stack.top *is* only updated when p2 is successfully
> shrunk, what if *only the p1 succeeds*?

In this case, would it be better to allocate both sess_map and stack.arr
in a single block, so we only call realloc once?

> My hunch is that you may still need some basic training on C
> programming.

I'd also be happy to to go through a basic training session on C too! :3

> Anyway, this patch is doing too many things. It's not something you
> should do in a single commit. Please split it into smaller, more
> manageable pieces.

Alright, I will try to split the patches.

--
Ahmad Gani