[PATCH 04/11] Port: removed useless msg->cancelled == 0 checks.

Alejandro Colomar alx.manpages at gmail.com
Fri Jun 17 09:34:03 UTC 2022


Hi Andrew,

On 6/17/22 04:46, Andrew Clayton wrote:
> On Thu, 16 Jun 2022 18:25:33 +0100
> Andrew Clayton <andrew at digital-domain.net> wrote:
> 
>> On Thu, 16 Jun 2022 17:51:30 +0200
>> Alejandro Colomar <alx.manpages at gmail.com> wrote:
>>
>>> This patch makes sense, but then, I guess we should go further and
>>> remove that field completely?  I don't see it being used at all.
>>
>> Yes, I think you're right, I'll check again and add it to the patch.
> 
> So, this has led down a bit of a rabbit hole!
> 
> ->cancelled is still used by
> 
> 1196         fmsg = nxt_port_frag_find(task, port, msg);
> ...
> 1202         if (nxt_fast_path(fmsg->cancelled == 0)) {
> 
> in src/nxt_port_socket.c::nxt_port_read_msg_process()
> 
>  From what I can tell fmsg is basically a copy of msg and stored in a
> hash table.
> 
> So how is msg created? Seems to be generally just created and
> set directly on the stack e.g
> 
[...]
> 
> However it doesn't seem to be zeroed out or anything and so unset
> fields will just contain whatever happens to be in memory.

Let me share my discoveries, maybe this helps you continue down the 
rabbit hole (I'm sure more than one point in this path is UB, but I'm 
not fixing it until I fully understand it; maybe you get it before me):


$ grepc nxt_port_recv_msg_t
./src/nxt_main.h:18:
typedef struct nxt_port_recv_msg_s   nxt_port_recv_msg_t;


$ grepc nxt_port_recv_msg_s
./src/nxt_port.h:194:
struct nxt_port_recv_msg_s {
     ...
     nxt_bool_t          cancelled;
     ...
};


$ grepc -tfd nxt_port_read_msg_process
./src/nxt_port_socket.c:1200:
static void
nxt_port_read_msg_process(nxt_task_t *task, nxt_port_t *port,
     nxt_port_recv_msg_t *msg)
{
     ...
     nxt_port_recv_msg_t  *fmsg;

     ...

         fmsg = nxt_port_frag_find(task, port, msg);

         ...

         if (nxt_fast_path(fmsg->cancelled == 0)) {

     ...
}


$ grepc nxt_port_frag_find
./src/nxt_port_socket.c:1161:
static nxt_port_recv_msg_t *
nxt_port_frag_find(nxt_task_t *task, nxt_port_t *port, 
nxt_port_recv_msg_t *msg)
{
     ...
     nxt_lvlhsh_query_t   lhq;
     ...

     res = last != 0 ? nxt_lvlhsh_delete(&port->frags, &lhq) :
           nxt_lvlhsh_find(&port->frags, &lhq);

     ...
         return lhq.value;

     ...
}


$ grepc nxt_lvlhsh_query_t
./src/nxt_lvlhsh.h:11:
typedef struct nxt_lvlhsh_query_s  nxt_lvlhsh_query_t;


$ grepc nxt_lvlhsh_query_s
./src/nxt_lvlhsh.h:88:
struct nxt_lvlhsh_query_s {
     ...
     void                      *value;

     ...
};


$ grepc -tfd nxt_lvlhsh_find
./src/nxt_lvlhsh.c:180:
nxt_int_t
nxt_lvlhsh_find(nxt_lvlhsh_t *lh, nxt_lvlhsh_query_t *lhq)
{
     ...
             return nxt_lvlhsh_bucket_find(lhq, slot);
         }

         return nxt_lvlhsh_level_find(lhq, slot, lhq->key_hash, 0);
     ...
}


$ grepc -tfd nxt_lvlhsh_bucket_find
./src/nxt_lvlhsh.c:227:
static nxt_int_t
nxt_lvlhsh_bucket_find(nxt_lvlhsh_query_t *lhq, void **bkt)
{
     ...
         bucket = nxt_lvlhsh_bucket(lhq->proto, bkt);
         n = nxt_lvlhsh_bucket_entries(lhq->proto, bkt);
         e = bucket;

         ...

                     value = nxt_lvlhsh_entry_value(e);

                     if (lhq->proto->test(lhq, value) == NXT_OK) {
                         lhq->value = value;
     ...
}


$ grepc nxt_lvlhsh_entry_value
./src/nxt_lvlhsh.c:103:
#define nxt_lvlhsh_entry_value(e) 
       \
     (void *) (((uintptr_t) (e)[1] << 32) + (e)[0])


./src/nxt_lvlhsh.c:125:
#define nxt_lvlhsh_entry_value(e) 
       \
     (void *) (e)[0]


(Above, the first definition is for 64 bit archs, the other is for else.)


$ grepc nxt_lvlhsh_bucket
./src/nxt_lvlhsh.c:78:
#define nxt_lvlhsh_bucket(proto, bkt) 
       \
     (uint32_t *) ((uintptr_t) bkt & ~(uintptr_t) proto->bucket_mask)


$ grepc nxt_lvlhsh_bucket_entries
./src/nxt_lvlhsh.c:82:
#define nxt_lvlhsh_bucket_entries(proto, bkt) 
       \
     (((uintptr_t) bkt & (uintptr_t) proto->bucket_mask) >> 1)


So my impression is that it's being used, although through the darkest 
of magics (uintptr_t), and I can't really follow it more to find the 
rabbit; maybe you do.
I hope this helps.

I wonder how the world did this without grepc(1) :p

Cheers,

Alex


-- 
Alejandro Colomar
<http://www.alejandro-colomar.es/>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://mailman.nginx.org/pipermail/unit/attachments/20220617/0449c141/attachment.bin>


More information about the unit mailing list