Remove Global/Static variables in path.c to make it thread safe #5148

ahmed-kamal2004 · 2025-05-13T19:11:58Z

Your checklist for this pull request

I've read the guidelines for contributing to this repository.
I made sure to follow the project's coding style.
I've documented every RZ_API function and struct this PR changes.
I've added tests that prove my changes are effective (required for changes to RZ_API).
I've updated the Rizin book with the relevant information (if needed).

Detailed description

Remove global/static variables in util/path.c, so removed the following:

portable_prefix
portable_prefix_searched
portable_prefix_mutex
and merged all of them to a struct RzPathPortable in include/rz_util.h

codecov · 2025-05-13T20:16:04Z

Codecov Report

Attention: Patch coverage is 74.28571% with 9 lines in your changes missing coverage. Please review.

Project coverage is 44.88%. Comparing base (144b47d) to head (a86059f).

Files with missing lines	Patch %	Lines
librz/util/path.c	77.27%	3 Missing and 2 partials ⚠️
librz/main/rizin.c	55.55%	4 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
librz/core/cconfig.c	`81.73% <100.00%> (ø)`
librz/core/core.c	`59.43% <100.00%> (+0.05%)`	⬆️
librz/egg/egg_lang.c	`37.58% <100.00%> (ø)`
librz/main/rizin.c	`49.89% <55.55%> (ø)`
librz/util/path.c	`57.89% <77.27%> (+4.66%)`	⬆️

... and 10 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 144b47d...a86059f. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

librz/include/rz_util/rz_path.h

wargio · 2025-05-14T00:36:50Z

librz/util/path.c

-static char *portable_prefix = NULL;
-static bool portable_prefix_searched = false;
-static RzThreadLock *portable_prefix_mutex = NULL;
+RzPathPortable *rz_portable;


Remove this global variable and add the struct to RzCore.

Remove constructor/destructor and it's header file and just initialize/finalize RzPath within RzCore init/fini function

wargio · 2025-05-14T00:37:29Z

librz/util/path.c

@@ -92,15 +88,16 @@ static char *set_portable_prefix(void) {
 */
 RZ_API void rz_path_set_prefix(RZ_NONNULL const char *path) {


Add RzPath as argument of this function

wargio · 2025-05-14T00:37:45Z

librz/include/rz_util/rz_path.h

+	char *prefix;
+	bool prefix_searched;
+	RzThreadLock *prefix_mutex;
+} RzPathPortable;


Call this just RzPath and make it a private structure. (see how RzDiff is defined)

wargio · 2025-05-14T00:39:53Z

librz/util/path.c

+	}
+	p->prefix = NULL;
+	p->prefix_searched = false;
+	p->prefix_mutex = rz_th_lock_new(recursive);


Pass false instead of an argument like you did.

wargio · 2025-05-14T00:40:07Z

librz/util/path.c

+ * \brief Return new RzPathPortable
+ * \return New RzPathPortable
+ */
+RZ_API RZ_OWN RzPathPortable *rz_path_portable_new(bool recursive) {


Remove the argument

wargio · 2025-05-14T00:40:25Z

librz/util/path.c

+		return NULL;
+	}
+	RZ_FREE(p->prefix);
+	p->prefix_searched = false;


No need, remove this line

wargio · 2025-05-14T00:42:00Z

librz/util/path.c

+	if (!p) {
+		return NULL;
+	}
+	RZ_FREE(p->prefix);


Call free. Use RZ_FREE/RZ_FREE_CUSTOM only if you intend to reuse the variables immediately afterward.

If the structure is then freed, there is no need to waste time setting them to NULL like these macros do

wargio · 2025-05-14T00:42:29Z

librz/util/path.c

+ * \brief Deallocate memory associated with a RzPathPortable object
+ * \return NULL
+ */
+RZ_API RZ_OWN RzPathPortable *rz_path_portable_free(RzPathPortable *p) {


Return void

wargio · 2025-05-14T00:42:43Z

librz/util/path.c

+	p->prefix_searched = false;
+	RZ_FREE_CUSTOM(p->prefix_mutex, rz_th_lock_free);
+	free(p);
+	return NULL;


Remove this.

ahmed-kamal2004 · 2025-05-14T23:43:43Z

Not Ready for merging yet

wargio

Good progress

wargio · 2025-05-15T12:42:15Z

librz/egg/egg_lang.c

For this file, just create it's own path struct within RzEgg like you did for RzCore

wargio · 2025-05-15T12:44:18Z

librz/include/rz_util/rz_path.h

+
+#ifdef RZ_API
+
+RZ_API void rz_path_set_prefix(RzPath *sys_path, RZ_NONNULL const char *path);


add RZ_NONNULL like for path and check the pointer in the assert (rz_return_if_fail) with the function.

wargio · 2025-05-15T12:44:27Z

librz/include/rz_util/rz_path.h

+#ifdef RZ_API
+
+RZ_API void rz_path_set_prefix(RzPath *sys_path, RZ_NONNULL const char *path);
+RZ_API RZ_OWN char *rz_path_prefix(RzPath *sys_path, RZ_NULLABLE const char *path);


wargio · 2025-05-15T12:45:14Z

librz/include/rz_util/rz_path.h

@@ -30,6 +39,11 @@ RZ_API RZ_OWN char *rz_path_home_expand(RZ_NULLABLE const char *path);

 RZ_API RZ_OWN char *rz_path_realpath(RZ_NULLABLE const char *path);

+RZ_API RZ_OWN RzPath *rz_path_new(void);
+RZ_API void rz_path_free(RzPath *p);


Here add RZ_NULLABLE but you don't need to add any assert

wargio · 2025-05-15T12:45:53Z

librz/util/path.c

@@ -12,28 +12,6 @@
 #include <rz_constructor.h>


Remove this also

wargio · 2025-05-15T12:46:27Z

librz/util/path.c

 * \param path Path to put in the install prefix context or NULL to just get the install prefix
 * \return \p path prefixed by the Rizin install prefix or just the install prefix
 */
-RZ_API RZ_OWN char *rz_path_prefix(RZ_NULLABLE const char *path) {
+RZ_API RZ_OWN char *rz_path_prefix(RzPath *sys_path, RZ_NULLABLE const char *path) {


wargio · 2025-05-15T12:47:00Z

librz/util/path.c

@@ -135,28 +115,28 @@ RZ_API RZ_OWN char *rz_path_prefix(RZ_NULLABLE const char *path) {
 * \brief Return the directory where include files are placed
 */
 RZ_API RZ_OWN char *rz_path_incdir(void) {


These functions needs RzPath as argument.

Well, I am doing iterations in the process, still need to do a lot of work for rz_path_system also

wargio · 2025-05-15T12:47:35Z

librz/util/path.c

 */
-RZ_API RZ_OWN char *rz_path_system_rc(void) {
-	return rz_path_prefix(RZ_GLOBAL_RC);
+RZ_API RZ_OWN char *rz_path_system_rc(RzPath *sys_path) {


wargio · 2025-05-15T12:48:33Z

librz/util/path.c

+ * \brief Deallocate memory associated with a RzPath pointer
+ * \return NULL
+ */
+RZ_API void rz_path_free(RzPath *p) {


RZ_NULLABLE

wargio · 2025-05-15T12:49:16Z

librz/util/path.c

+ * \return NULL
+ */
+RZ_API void rz_path_free(RzPath *p) {
+	if (p) {


Invert the if.

if (!p) { return; }

Ok, for code readability?

wargio · 2025-05-16T12:01:07Z

binrz/rz-test/rz-test.c

@@ -192,7 +192,7 @@ int rz_test_main(int argc, const char **argv) {
 	int ret = 0;
 	char *cwd = NULL;
 	RzTestState state = { 0 };
-
+	RzPath *sys_path = rz_path_new();


Check for null.

wargio · 2025-05-16T12:02:28Z

librz/core/tui/panels.c

@@ -503,7 +503,7 @@ static int __watch_points_cb(void *user);
 static int __references_cb(void *user);
 static int __help_cb(void *user);
 static int __fortune_cb(void *user);
-static int __version_cb(void *user);
+static int __version_cb(RzPath *sys_path, void *user);


I think that user is RzCore.

Rot127

I would advice to add a test already for the changes made. The more edits you make without tests, the harder it will be to handle the complexity later.
Also, Coverity will be happy :)

ahmed-kamal2004 added 3 commits May 13, 2025 22:05

Add struct in path.c

5458e15

Add RzPathPortable initializer and deallocator funcs

762576e

Add rz_return_if_fail

a0d8b23

ahmed-kamal2004 requested review from ret2libc, thestr4ng3r, wargio, kazarmy and Rot127 as code owners May 13, 2025 19:11

github-actions bot added API RzUtil labels May 13, 2025

wargio requested changes May 14, 2025

View reviewed changes

Use init & fini of RzCore

491b5f7

github-actions bot added rizin RzCore RzEgg labels May 14, 2025

Rename rz_path to sys_path

a86059f

wargio requested changes May 15, 2025

View reviewed changes

Add RzPath* to multiple functions in path.c

a1ffa69

ahmed-kamal2004 requested a review from yossizap as a code owner May 16, 2025 05:19

github-actions bot added rz-asm rz-bin rz-test RzLang rz-ax visual panel rz-run RzSocket labels May 16, 2025

wargio reviewed May 16, 2025

View reviewed changes

Rot127 reviewed May 16, 2025

View reviewed changes

		@@ -92,15 +88,16 @@ static char *set_portable_prefix(void) {
		*/
		RZ_API void rz_path_set_prefix(RZ_NONNULL const char *path) {


		#ifdef RZ_API

		RZ_API void rz_path_set_prefix(RzPath sys_path, RZ_NONNULL const char path);

Uh oh!

Remove Global/Static variables in path.c to make it thread safe #5148

Are you sure you want to change the base?

Remove Global/Static variables in path.c to make it thread safe #5148

Uh oh!

Conversation

ahmed-kamal2004 commented May 13, 2025

Uh oh!

codecov bot commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahmed-kamal2004 commented May 14, 2025

Uh oh!

wargio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rot127 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented May 13, 2025 •

edited

Loading

Rot127 left a comment •

edited

Loading