STACK: Change PATH_MAX stack allocation to heap #10127

michal-shalev · 2024-09-05T11:16:36Z

What?

This PR changes all stack allocations of char arrays sized PATH_MAX to heap allocations.

Why?

Customers with limited stack sizes have recently experienced stack overflow issues when running UCX. Moving these allocations to the heap prevents such overflows.

How?

Heap allocations replace stack allocations for char arrays of size PATH_MAX.

src/tools/vfs/vfs_main.c

ivankochin · 2024-09-18T06:41:00Z

src/tools/vfs/vfs_main.c

    }

    /* Remove mount point directory */
    vfs_log("removing directory '%s'", mountpoint);
    ret = rmdir(mountpoint);
    if (ret < 0) {
        vfs_error("failed to remove directory '%s': %m", mountpoint);
-        return ret;
+        goto out_free_mountpoint;


Why need goto?

removed it, thanks, done

src/ucs/config/parser.c

ivankochin · 2024-09-18T06:45:01Z

src/ucs/debug/memtrack.c

@@ -433,10 +433,10 @@ void ucs_memtrack_init()
        return;
    }

+    ucs_memtrack_vfs_init();


Is that change related to this PR?

yes, I changed allocation at VFS to heap and it revealed this issue - an invalid count in the memtrack (tests were failing)

src/ucs/vfs/base/vfs_obj.c

ivankochin · 2024-09-18T07:01:14Z

src/ucs/vfs/sock/vfs_sock.c

    ret = mkdir(dirname(sock_path_dir), S_IRWXU);
    if ((ret < 0) && (errno != EEXIST)) {
        ucs_log(log_level, "failed to create directory '%s': %m",
                sock_path_dir);
-        return -errno;
+        ret = -errno;
+        goto out_free_sock_path_dir;


Why need goto?

removed it, thanks, done

src/uct/tcp/tcp_iface.c

src/tools/vfs/vfs_main.c

src/ucs/config/parser.c

yosefe · 2024-10-21T10:50:01Z

src/ucs/config/parser.c

+    status = ucs_string_alloc_path_buffer(&path, "path");
+    if (status != UCS_OK) {
+        return;
+    }


this can be moved inside the "if" block in lines 1780..1782 (along with ucs_free)?

there are multiple places where ucs_string_alloc_path_buffer is used along with "strncpy" + "dirname", can we unite them to a single function that allocates a dirname? char *ucs_string_get_dirname(const char *path) that returns an allocated directory path?

src/ucs/sys/module.c

src/ucs/sys/string.h

src/ucs/sys/topo/base/topo.c

src/uct/sm/mm/posix/mm_posix.c

yosefe · 2024-10-21T13:38:59Z

src/uct/sm/mm/posix/mm_posix.c

+                                         fd_p);
+
+    ucs_free(file_path);
+out:


minor: space line before

src/uct/sm/mm/posix/mm_posix.c

yosefe · 2024-10-22T07:55:46Z

src/ucs/sys/topo/base/topo.c

@@ -445,6 +445,7 @@ ucs_topo_get_distance_sysfs(ucs_sys_device_t device1,
    }

    /* Report best perf for common PCI bridge or sysfs parsing error */
+    status = ucs_topo_get_distance_default(device1, device2, distance);


now can we remove line 408 and goto here (restore distance_default label)?

Restored it, please check now

src/ucs/sys/module.c

src/ucs/config/parser.c

src/ucs/sys/string.c

src/ucs/sys/topo/base/topo.c

src/ucs/vfs/fuse/vfs_fuse.c

src/ucs/sys/string.c

src/ucs/sys/topo/base/topo.c

michal-shalev requested review from yosefe and gleon99 September 5, 2024 11:16

michal-shalev force-pushed the change-path-max-stack-alloc-to-heap branch from 8cb5ec5 to 2149869 Compare September 5, 2024 11:17

michal-shalev marked this pull request as draft September 5, 2024 11:31

michal-shalev added the WIP-DNM Work in progress / Do not review label Sep 5, 2024

michal-shalev changed the title ~~Change path max stack alloc to heap~~ Change path max stack allocation to heap Sep 5, 2024

michal-shalev removed request for yosefe and gleon99 September 5, 2024 23:27

michal-shalev removed the WIP-DNM Work in progress / Do not review label Sep 5, 2024

michal-shalev marked this pull request as ready for review September 5, 2024 23:27

michal-shalev requested review from gleon99 and yosefe September 6, 2024 00:26

michal-shalev force-pushed the change-path-max-stack-alloc-to-heap branch 2 times, most recently from bbc428e to 92fdb9a Compare September 10, 2024 09:03

michal-shalev marked this pull request as draft September 10, 2024 13:12

michal-shalev added the WIP-DNM Work in progress / Do not review label Sep 10, 2024

michal-shalev removed request for gleon99 and yosefe September 10, 2024 13:12

michal-shalev force-pushed the change-path-max-stack-alloc-to-heap branch from 92fdb9a to 4920c1e Compare September 16, 2024 10:49

michal-shalev requested a review from yosefe September 16, 2024 10:49

michal-shalev marked this pull request as ready for review September 16, 2024 10:49

michal-shalev force-pushed the change-path-max-stack-alloc-to-heap branch 6 times, most recently from 66bbe34 to 27e63b9 Compare September 16, 2024 17:30

michal-shalev removed the WIP-DNM Work in progress / Do not review label Sep 17, 2024

ivankochin reviewed Sep 18, 2024

View reviewed changes

michal-shalev changed the title ~~Change path max stack allocation to heap~~ BUILD: Change path max stack allocation to heap Sep 18, 2024

michal-shalev dismissed Artemy-Mellanox’s stale review via 9f41e2d October 10, 2024 12:17

michal-shalev force-pushed the change-path-max-stack-alloc-to-heap branch from 5ec4d39 to 9f41e2d Compare October 10, 2024 12:17

yosefe reviewed Oct 16, 2024

View reviewed changes

src/uct/tcp/tcp_iface.c Outdated Show resolved Hide resolved

src/tools/vfs/vfs_main.c Outdated Show resolved Hide resolved

src/ucs/config/parser.c Outdated Show resolved Hide resolved

michal-shalev force-pushed the change-path-max-stack-alloc-to-heap branch 2 times, most recently from 8510c36 to 54cbfe1 Compare October 20, 2024 21:50

michal-shalev requested a review from Artemy-Mellanox October 20, 2024 21:52

michal-shalev force-pushed the change-path-max-stack-alloc-to-heap branch from 54cbfe1 to 3b439c3 Compare October 20, 2024 22:48

michal-shalev requested a review from yosefe October 20, 2024 23:08

michal-shalev force-pushed the change-path-max-stack-alloc-to-heap branch from 3b439c3 to 5f64c2c Compare October 21, 2024 00:06

yosefe reviewed Oct 21, 2024

View reviewed changes

michal-shalev changed the title ~~Change path max stack allocation to heap~~ STACK: Change PATH_MAX stack allocation to heap Oct 21, 2024

yosefe reviewed Oct 22, 2024

View reviewed changes

michal-shalev requested a review from yosefe October 22, 2024 08:58

michal-shalev force-pushed the change-path-max-stack-alloc-to-heap branch 2 times, most recently from 5e1cce5 to 962125b Compare October 22, 2024 20:33

yosefe reviewed Oct 27, 2024

View reviewed changes

michal-shalev force-pushed the change-path-max-stack-alloc-to-heap branch 3 times, most recently from 553acc0 to 643c6e6 Compare October 27, 2024 22:32

michal-shalev requested a review from yosefe October 27, 2024 22:33

michal-shalev force-pushed the change-path-max-stack-alloc-to-heap branch from 643c6e6 to f04244e Compare October 28, 2024 09:02

yosefe reviewed Oct 31, 2024

View reviewed changes

src/ucs/sys/topo/base/topo.c Show resolved Hide resolved

michal-shalev requested a review from yosefe November 3, 2024 02:09

yosefe approved these changes Nov 3, 2024

View reviewed changes

STACK: Change PATH_MAX stack allocation to heap

e161c38

michal-shalev force-pushed the change-path-max-stack-alloc-to-heap branch from cacaf3f to e161c38 Compare November 3, 2024 17:02

michal-shalev enabled auto-merge November 3, 2024 17:02

michal-shalev added Approved pending CI and removed Ready for Review labels Nov 3, 2024

michal-shalev merged commit 68a742f into openucx:master Nov 4, 2024
141 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STACK: Change PATH_MAX stack allocation to heap #10127

STACK: Change PATH_MAX stack allocation to heap #10127

michal-shalev commented Sep 5, 2024 •

edited

Loading

ivankochin Sep 18, 2024

michal-shalev Sep 22, 2024

ivankochin Sep 18, 2024

michal-shalev Sep 18, 2024

ivankochin Sep 18, 2024

michal-shalev Sep 22, 2024

yosefe Oct 21, 2024

michal-shalev Oct 22, 2024

yosefe Oct 21, 2024

michal-shalev Oct 21, 2024

yosefe Oct 22, 2024

michal-shalev Oct 22, 2024

STACK: Change PATH_MAX stack allocation to heap #10127

STACK: Change PATH_MAX stack allocation to heap #10127

Conversation

michal-shalev commented Sep 5, 2024 • edited Loading

What?

Why?

How?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michal-shalev commented Sep 5, 2024 •

edited

Loading