Follow-up Comment #2, bug #24171 (project freeciv):

Looking at disassembly, I think this is caused when dereferencing
pc->phs.handlers.


int send_packet_chat_msg_req(struct connection *pc, const struct
packet_chat_msg_req *packet)
{
  if(!pc->used) {
    log_error("WARNING: trying to send data to the closed connection %s",
              conn_description(pc));
    return -1;
  }
  fc_assert_ret_val_msg(pc->phs.handlers->send[PACKET_CHAT_MSG_REQ].packet !=
NULL, -1,
                        "Handler for PACKET_CHAT_MSG_REQ not installed");
  return pc->phs.handlers->send[PACKET_CHAT_MSG_REQ].packet(pc, packet);
}


I think this pointer could be invalid when packet_handlers_free() (which is an
atexit() handler) has been called before the client's at_exit() gets to run.

Since atexit handlers are called in reverse order of registration, and the
client at_exit() is registered at client startup whereas the packets.c
packet_handlers_free() is registered on demand and I think that's on making a
connection to the server for the first time, it looks inevitable to me that
at_exit() will reference freed storage (unless atexit() doesn't call
client_kill_server(), but on quitting a running game I think it always will).

Running the current S2_6 client on Linux under valgrind confirms this
diagnosis.

This packet_handlers hash and atexit handler is new on S2_6 (patch #5565).

It's probably a bad idea to have dependencies between atexit() handlers.
Probably packet_handlers_free() should be called explicitly from somewhere.
Not sure where.

Disassembly of the Windows build:


0056b8dc <_send_packet_chat_msg_req>:
  56b8dc:       55                      push   %ebp
  56b8dd:       89 e5                   mov    %esp,%ebp
  56b8df:       53                      push   %ebx
  56b8e0:       83 ec 24                sub    $0x24,%esp
  56b8e3:       8b 5d 08                mov    0x8(%ebp),%ebx
  56b8e6:       8b 55 0c                mov    0xc(%ebp),%edx
  56b8e9:       80 7b 08 00             cmpb   $0x0,0x8(%ebx)
  56b8ed:       74 1d                   je     56b90c
<_send_packet_chat_msg_req   # => log_error()
  56b8ef:       8b 83 ac 06 00 00       mov    0x6ac(%ebx),%eax
* 56b8f5:       8b 40 68                mov    0x68(%eax),%eax
  56b8f8:       85 c0                   test   %eax,%eax
  56b8fa:       74 24                   je     56b920
<_send_packet_chat_msg_req   # => fc_assert_fail()


Valgrind memcheck output for S2_6 r30795 on Linux amd64:


Invalid read of size 8
   at 0x5EC325: send_packet_chat_msg_req (packets_gen.c:4504)
   by 0x5EC3F5: dsend_packet_chat_msg_req (packets_gen.c:4515)
   by 0x475909: client_kill_server (connectdlg_common.c:150)
   by 0x47332D: emergency_exit (client_main.c:223)
   by 0x47332D: at_exit (client_main.c:231)
   by 0x76F7258: __run_exit_handlers (exit.c:82)
   by 0x76F72A4: exit (exit.c:104)
   by 0x473A05: client_exit (client_main.c:727)
   by 0x473D9C: client_main (client_main.c:670)
   by 0x76DCEC4: (below main) (libc-start.c:287)
 Address 0x1a13a790 is 208 bytes inside a block of size 4,000 free'd
   at 0x4C2BDEC: free (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x61B4A6: genhash_slot_free (genhash.c:483)
   by 0x61B4A6: genhash_clear (genhash.c:595)
   by 0x61B531: genhash_destroy (genhash.c:298)
   by 0x5642A4: packet_handler_hash_destroy (spechash.h:419)
   by 0x5642A4: packet_handlers_free (packets.c:806)
   by 0x76F7258: __run_exit_handlers (exit.c:82)
   by 0x76F72A4: exit (exit.c:104)
   by 0x473A05: client_exit (client_main.c:727)
   by 0x473D9C: client_main (client_main.c:670)
   by 0x76DCEC4: (below main) (libc-start.c:287)


    _______________________________________________________

Reply to this item at:

  <http://gna.org/bugs/?24171>

_______________________________________________
  Message sent via/by Gna!
  http://gna.org/


_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to