Skip to content

Bug fixes for ADB #45

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JianyuWang0623 wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Bug fixes for ADB #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
243f21e
microadb: fix used after free issue about handle close
Donny9 Nov 29, 2023
6b47b63
microADB: fix memory leaks when adbd exit
Donny9 Nov 30, 2023
262457c
microADB: call service_close to send CLSE frame when child task exit
Donny9 Dec 14, 2023
25236f0
microADB: avoid kick when read/write pipe had beed close
Donny9 Dec 14, 2023
e70f524
Send response before adb_reboot_impl()
JianyuWang0623 Jan 5, 2024
84f46c0
Check whether client has services before close
JianyuWang0623 Jan 10, 2024
868a51c
adb/microadb: Copy packet to svc to prevent the current packet from b…
Donny9 Jan 15, 2024
0e3390d
adb:do not send sigkill when shell close
guohao15 Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions adb_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
#include "tcp_service.h"
#endif

#define REBOOT_SERVICE ((adb_service_t *)(~0ul))

/****************************************************************************
* Private Function Prototypes
****************************************************************************/
Expand Down Expand Up @@ -152,7 +154,7 @@ static void handle_open_frame(adb_client_t *client, apacket *p) {
else {
send_close_frame(client, p, 0, p->msg.arg0);
}
} else {
} else if (svc != REBOOT_SERVICE) {
if (p->write_len == APACKET_SERVICE_INIT_ASYNC) {
/* Service init is asynchronous. Release apacket. */
adb_hal_apacket_release(client, p);
Expand Down Expand Up @@ -383,10 +385,9 @@ static adb_service_t *adb_service_open(adb_client_t *client, const char *name, a
#endif

if (!strncmp(name, "reboot:", 7)) {
adb_send_okay_frame(client, p, client->next_service_id++, p->msg.arg0);
adb_reboot_impl(&name[7]);

/* One shot service, skip service register */
return NULL;
return REBOOT_SERVICE;
}
} while (0);

Expand All @@ -408,7 +409,7 @@ void adb_service_close(adb_client_t *client, adb_service_t *svc, apacket *p) {
goto exit_free_service;
}

while (cur_svc->next) {
while (cur_svc != NULL && cur_svc->next != NULL) {
if (cur_svc->next == svc) {
cur_svc->next = svc->next;
goto exit_free_service;
Expand Down
36 changes: 32 additions & 4 deletions file_sync_service.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ enum {
typedef struct afs_service_s {
adb_service_t service;
uint8_t *packet_ptr;
uint8_t *payload;

uint8_t state;
unsigned cmd;
Expand Down Expand Up @@ -262,6 +263,23 @@ static int create_path_directories(char *name)
return 0;
}

static uint8_t *get_payload(afs_service_t *svc, apacket *p)
{
if (svc->size > 0) {
if (svc->payload == NULL) {
svc->payload = malloc(CONFIG_ADBD_PAYLOAD_SIZE);
if (svc->payload == NULL) {
return NULL;
}
}

memcpy(svc->payload, p->data, p->msg.data_length);
return svc->payload;
}

return p->data;
}

static void state_reset(afs_service_t *svc)
{
switch (svc->state) {
Expand Down Expand Up @@ -730,7 +748,7 @@ static int state_wait_cmd_data(afs_service_t *svc, apacket *p)
static int file_sync_on_write(adb_service_t *service, apacket *p) {
int ret = 0;
afs_service_t *svc = container_of(service, afs_service_t, service);
svc->packet_ptr = p->data;
svc->packet_ptr = get_payload(svc, p);

/* Process all packet data */

Expand Down Expand Up @@ -779,7 +797,7 @@ static int file_sync_on_write(adb_service_t *service, apacket *p) {
static int file_sync_on_ack(adb_service_t *service, apacket *p) {
int ret;
afs_service_t *svc = container_of(service, afs_service_t, service);
svc->packet_ptr = p->data;
svc->packet_ptr = get_payload(svc, p);

/* No data in notify packet */
switch (svc->state) {
Expand All @@ -791,11 +809,19 @@ static int file_sync_on_ack(adb_service_t *service, apacket *p) {
ret = state_process_list(svc, p);
break;

case AFS_STATE_PROCESS_SEND_FILE_DATA:
case AFS_STATE_PROCESS_SEND_FILE_HDR:
case AFS_STATE_PROCESS_SEND_SYM_HDR:
case AFS_STATE_WAIT_CMD_DATA:
case AFS_STATE_WAIT_CMD:
/* Nothing to do */
ret = 0;
/* Since the WRITE frame can contain multiple incomplete
* combinations of ID_SEND and ID_DONE, when the pc replies to ID_DONE,
* the OKAY frame sent can be at any time in the state machine.
* At this time, we should not reset the state machine and continue
* to process the next frame status.
*/

ret = 1;
break;

default:
Expand All @@ -817,6 +843,7 @@ static int file_sync_on_ack(adb_service_t *service, apacket *p) {
static void file_sync_on_close(struct adb_service_s *service) {
afs_service_t *svc = container_of(service, afs_service_t, service);
state_reset(svc);
free(svc->payload);
free(svc);
}

Expand All @@ -842,6 +869,7 @@ adb_service_t* file_sync_service(const char *params)
}

service->size = 0;
service->payload = NULL;
service->state = AFS_STATE_WAIT_CMD;
service->service.ops = &file_sync_ops;

Expand Down
5 changes: 4 additions & 1 deletion hal/hal_uv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,10 @@ adb_context_t* adb_hal_create_context(void) {
}

void adb_hal_destroy_context(adb_context_t *context) {
UNUSED(context);
adb_context_uv_t *adbd =
container_of(context, adb_context_uv_t, context);

uv_loop_close(adbd->loop);

@spiriou spiriou Oct 21, 2024

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.

@JianyuWang0623 JianyuWang0623 Oct 24, 2024

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);

}

adb_client_t *adb_hal_create_client(size_t size) {
Expand Down
6 changes: 5 additions & 1 deletion hal/hal_uv_client_usb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ static int usb_uv_write(adb_client_t *c, apacket *p) {
static void usb_uv_kick(adb_client_t *c) {
adb_client_usb_t *client = container_of(c, adb_client_usb_t, uc.client);

if (uv_is_closing((uv_handle_t*)&client->read_pipe)) {
return;
}

if (!uv_is_active((uv_handle_t*)&client->read_pipe)) {
/* Restart read events */
int ret = uv_read_start((uv_stream_t*)&client->read_pipe,
Expand All @@ -111,8 +115,8 @@ static void usb_uv_close(adb_client_t *c) {
adb_client_usb_t *client = (adb_client_usb_t*)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);

@spiriou spiriou Oct 21, 2024

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?

}

static const adb_client_ops_t adb_usb_uv_ops = {
Expand Down
4 changes: 2 additions & 2 deletions hal/shell_service_uv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,9 @@ static int shell_set_cloexec(int fd) {
static void on_child_exit(uv_process_t *process, int64_t exit_status,
int term_signal) {
ash_service_t *svc = (ash_service_t*)process->data;
adb_client_uv_t *client = (adb_client_uv_t *)svc->shell_pipe.data;

adb_service_close(&client->client, &svc->service, NULL);
adb_log("shell %d<->%d exited with status %ld, signal %d\n",
svc->service.id, svc->service.peer_id,
exit_status, term_signal);
Expand Down Expand Up @@ -219,8 +221,6 @@ static void shell_close(adb_service_t *service) {

/* Terminate child process in case it is still running */

uv_process_kill(&svc->process, SIGKILL);

uv_close((uv_handle_t *)&svc->shell_pipe, shell_close_pipe_callback);
}

Expand Down