Skip to content

Bug fixes for ADB - #45

Open
JianyuWang0623 wants to merge 8 commits into
spiriou:masterfrom
JianyuWang0623:br_wjy_microadb_bugfix_1014
Open

Bug fixes for ADB#45
JianyuWang0623 wants to merge 8 commits into
spiriou:masterfrom
JianyuWang0623:br_wjy_microadb_bugfix_1014

Conversation

@JianyuWang0623

Copy link
Copy Markdown
Contributor

Summary

  1. Fix used after free issue about handle close - 243f21e
  2. Fix memory leaks when adbd exit - 6b47b63
  3. Call service_close to send CLSE frame when child task exit - 262457c
  4. Avoid kick when read/write pipe had beed close - 25236f0
  5. Send response before adb_reboot_impl() to avoid host reporting error log - e70f524
  6. Check whether client has services before close to fix crash - 84f46c0
  7. Copy packet to svc to prevent the current packet from being overwritten - 868a51c
  8. Avoid sending termination signals to tasks that are accessing the file system to avoid deadlock - 0e3390d

Impact

Bugfix for microADB.

Test

  • Selftest
  • CI

Donny9 and others added 8 commits October 14, 2024 00:18
uv__run_closing_handles libuv/src/unix/core.c:365
uv_run libuv/src/unix/core.c:464
adb_hal_run microADB/hal/hal_uv.c:69
adbd_main /home/djz/workspace/vela_rp/apps/system/adb/adb_main.c:157
nxtask_startup sched/task_startup.c:70
nxtask_start task/task_start.c:134
pre_start sim/sim_initialstate.c:52

Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>
host_memalign sim/posix/sim_hostmemory.c:168
host_realloc sim/posix/sim_hostmemory.c:193
mm_realloc sim/sim_heap.c:334
mm_malloc sim/sim_heap.c:272
mm_zalloc sim/sim_heap.c:402
mm_calloc sim/sim_heap.c:387
calloc umm_heap/umm_calloc.c:73
uv__calloc libuv/src/uv-common.c:94
uv_loop_init libuv/src/unix/loop.c:40
uv_default_loop libuv/src/uv-common.c:821
adb_hal_create_context microADB/hal/hal_uv.c:35
adbd_main /home/djz/workspace/vela_rp/apps/system/adb/adb_main.c:151
nxtask_startup sched/task_startup.c:70
nxtask_start task/task_start.c:134
pre_start sim/sim_initialstate.c:52

Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>
Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>
__assert
nuttx/libs/libc/assert/lib_assert.c:36
usb_uv_kick
apps/system/adb/microADB/hal/hal_uv_client_usb.c:197
adb_hal_apacket_release
/media/liangchaozhong/ssd/x4b/apps/system/adb/microADB/hal/hal_uv_packet.c:55
adb_uv_close_client
apps/system/adb/microADB/hal/hal_uv.c:100
usb_uv_on_close
apps/system/adb/microADB/hal/hal_uv_client_usb.c:209
uv__finish_close
apps/system/libuv/libuv/src/unix/core.c:352
adb_hal_run
apps/system/adb/microADB/hal/hal_uv.c:69
adbd_main

Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>
Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
…eing overwritten

When the windows/mac PC sends multiple files in a write packet, and the last DONE packet sent is incomplete,
the remaining bytes of DONE are in the next wirte packet, which will lead to the following two situations.

1. When the sync message has not been completely received, we should maintain the state machine state

current frame:
2f 63 6f 6d 2e 76 65 6c 61 2e 63 68 61 72 74 2e  /com.vela.chart.
64 65 6d 6f 2f 69 31 38 6e 2f 65 6e 2e 6a 73 6f  demo/i18n/en.jso
6e 2c 33 33 32 30 36 44 41 54 41 13 00 00 00 7b  n,33206DATA....{
22 61 22 3a 7b 22 62 22 3a 22 68 65 6c 6c 6f 22  "a":{"b":"hello"
7d 7d 44 4f 4e 45 88 e2                          }}DONE..

next frame:
57 52 54 45 3a 00 00 00 01 00 00 00 00 04 00 00  WRTE:...........
c6 ac 01 00 a8 ad ab ba 88 65 53 45 4e 44 43 00  .........eSENDC.
00 00 2f 64 61 74 61 2f 63 6f 6d 2e 76 65 6c 61  ../data/com.vela
2e 63 68 61 72 74 2e 64 65 6d 6f 2f 63 6f 6d 2e  .chart.demo/com.
76 65 6c 61 2e 63 68 61 72 74 2e 64 65 6d 6f 2f  vela.chart.demo/

The complete DONE package is: | 44 4f 4e 45 88 e2 | 88 65 |
                                   current          next

2. When the device receives DONE, it needs to reply OKAY to the HOST.If the current apcket is reused,
OKAY will overwrite the unprocessed content, so a local buffer needs to be used.

before wirte:

57 52 54 45 3a 00 00 00 01 00 00 00 00 04 00 00  WRTE:...........
c6 ac 01 00 a8 ad ab ba 88 65 53 45 4e 44 43 00  .........eSENDC.

after write:

57 52 54 45 3a 00 00 00 01 00 00 00 00 04 00 00  WRTE:...........
c6 ac 01 00 a8 ad ab ba 41 59 00 00 00 00 00 00  ........AY......
                        ^                    ^

the buffer ".eSENDC" -> "AY......"

Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>
Signed-off-by: guohao15 <guohao15@xiaomi.com>
@JianyuWang0623

Copy link
Copy Markdown
Contributor Author

Splited from #42.
CC: @xiaoxiang781216 @GUIDINGLI @Donny9 @guohao15

Comment thread hal/hal_uv_client_usb.c
/* Close pipe and cancel all pending write requests if any */
uv_close((uv_handle_t*)&client->write_pipe, NULL);
uv_close((uv_handle_t*)&client->read_pipe, usb_uv_on_close);
uv_close((uv_handle_t*)&client->write_pipe, NULL);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we move the uv_close for write_pipe in "usb_uv_on_close" to chain them?

Comment thread hal/hal_uv.c
adb_context_uv_t *adbd =
container_of(context, adb_context_uv_t, context);

uv_loop_close(adbd->loop);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That covers the case where uv_loop is initialized, but due to an error we exit without calling uv_loop_run() ?

Releases all internal loop resources. Call this function only when the loop has finished executing and all open handles and requests have been closed, or it will return UV_EBUSY. After this function returns, the user can free the memory allocated for the loop.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That covers the case where uv_loop is initialized, but due to an error we exit without calling uv_loop_run() ?

Releases all internal loop resources. Call this function only when the loop has finished executing and all open handles and requests have been closed, or it will return UV_EBUSY. After this function returns, the user can free the memory allocated for the loop.

As nuttx-apps/system/adb/adb_main.c shown, currently adb_hal_destroy_context() maybe always called after adb_hal_run()/uv_run().

// L159 ~ L160
  adb_hal_run(ctx);
  adb_hal_destroy_context(ctx);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants