WIP: Deprecate r_str_new and R_STR_DUP ##refactor #23119

satk0 · 2024-07-12T21:34:08Z

Mark this if you consider it ready to merge
I've added tests (optional)
I wrote some lines in the book (optional)

Description

Partially solves issue #20959

satk0 · 2024-07-12T21:39:59Z

I created a new macro R_STRDUP that aims to null check strdup.

I am thinking about creating another macro called R_STRNDUP aiming to replace r_str_ndup and r_str_newlen, wdyt?

trufae

use only R_STRDUP when we can't control if the string passed can be null, use strdup() instead of r_str_new. the idea behind R_STR_DUP instead of R_STRDUP was to follow the R_STR_ prefix like it is for r_str for consistency.

trufae · 2024-07-13T07:33:53Z

libr/arch/arch_value.c

@@ -108,7 +108,7 @@ R_API bool r_arch_value_set_ut64(RArchValue *val, RReg *reg, RIOBind *iob, ut64
 R_API char *r_arch_value_tostring(RArchValue *value) {
 	char *out = NULL;
 	if (value) {
-		out = r_str_new ("");
+		out = R_STRDUP ("");


this can be just strdup(), there's no need for a null check here

trufae · 2024-07-13T07:34:14Z

libr/arch/p/8051/8051_disas.c

@@ -31,7 +31,7 @@ static char *r_8051_disas(ut64 pc, const ut8 *buf, int len, int *olen) {
 				// op @Ri; op Rn
 				disasm = r_str_newf (name, buf[0] & mask);
 			} else {
-				disasm = r_str_new (name);
+				disasm = R_STRDUP (name);


i think name cant be null here

trufae · 2024-07-13T07:34:30Z

libr/arch/p/dalvik/plugin.c

@@ -761,9 +761,9 @@ static int dalvik_disassemble(RArchSession *as, RAnalOp *op, ut64 addr, const ut
 			R_FREE (strasm);
 			size = 2;
 		}
-		op->mnemonic = r_str_new (r_str_get_fail (strasm, "invalid"));
+		op->mnemonic = R_STRDUP (r_str_get_fail (strasm, "invalid"));


strdup no null check here

trufae · 2024-07-13T07:34:34Z

libr/arch/p/dalvik/plugin.c

 	} else if (len > 0) {
-		op->mnemonic = r_str_new ("invalid");
+		op->mnemonic = R_STRDUP ("invalid");


trufae · 2024-07-13T07:36:46Z

the story about strndup (and R_STR_NDUP) is because strndup is NOT portable. so that's why we have r_str_ndup and imho this function should be checking for null at ocmpile time with an R_RETURN statement

satk0 · 2024-07-13T09:16:57Z

Maybe I would just remove R_STRDUP and use R_STR_DUP for consistency as well as create new R_STR_NDUP macro that would do what you wrote above?

trufae · 2024-07-13T09:43:26Z

Sounds like a reasonable change 👍

satk0 · 2024-07-13T17:09:47Z

Don't look at that. I've just realized that I shouldn't be using strndup because it is not portable as you wrote. So may I just copy paste r_str_ndup as a R_STR_NDUP macro?

trufae · 2024-07-14T08:55:39Z

libr/anal/sign.c

@@ -343,7 +343,7 @@ R_API bool r_sign_deserialize(RAnal *a, RSignItem *it, const char *k, const char
 			if (token[0] != 0) {
 				it->hash = R_NEW0 (RSignHash);
 				if (it->hash) {
-					it->hash->bbhash = r_str_new (token);
+					it->hash->bbhash = strdup (token);


thats not the same, r_str_new checks for null and returns null. so the behaviour may differ because the code expects to have bbhash=NULL

R_API char *r_str_new(const char *str) { return str? strdup (str): NULL; }

trufae · 2024-07-14T08:55:43Z

libr/anal/sign.c

@@ -248,7 +248,7 @@ R_API bool r_sign_deserialize(RAnal *a, RSignItem *it, const char *k, const char
 	}

 	it->space = r_spaces_add (&a->zign_spaces, r_str_word_get0 (k2, 1));
-	it->name = r_str_new (r_str_word_get0 (k2, 2));
+	it->name = strdup (r_str_word_get0 (k2, 2));


Yeah, that's why it is better to be careful when making changes like this. Previously when I was digging with it I messed up to such an extend that even r2 -v segfaulted XD

satk0 · 2024-07-23T12:48:08Z

@trufae I finally managed to fix the CI/CD and I think it is ready to merge. The only thing I think about is to whether depreciate R_STR_DUP in favour of a new r_str_dup function (in lowercase) to be consistent with r_str_ndup, what do you think?

libr/arch/p/pyc/opcode_arg_fmt.c

trufae · 2024-07-24T17:26:31Z

shlr/ar/ar.c

-	return r_str_newlen (buf + off, i - off - 1);
+	if (i - off - 1) {
+		return r_str_ndup (buf + off, i - off - 1);
+	} else {


do not else after a return

trufae · 2024-07-24T17:26:43Z

libr/util/table.c

@@ -267,7 +267,7 @@ R_API void r_table_add_rowf(RTable *t, const char *fmt, ...) {
 			r_list_append (list, r_str_newf ("%.03lf", va_arg (ap, double)));
 			break;
 		case 'b':
-			r_list_append (list, r_str_new (r_str_bool (va_arg (ap, int))));
+			r_list_append (list, R_STR_DUP (r_str_bool (va_arg (ap, int))));


strdup is fine, this cant be null

trufae · 2024-07-24T17:26:58Z

libr/util/str.c

@@ -672,7 +672,10 @@ R_API char *r_str_trunc_ellipsis(const char *str, int len) {
 	if (strlen (str) < len) {
 		return strdup (str);
 	}
-	char *buf = r_str_newlen (str, len);


r_str_newlen is deprecated too, right?

maybe r_str_ndup should handle negative length in the same way or we shuol dhave a macro for that, as it seems the alternative is made in so many different places

trufae

.

trufae · 2024-07-27T06:03:53Z

libr/anal/sign.c

@@ -227,8 +227,8 @@ R_API bool r_sign_deserialize(RAnal *a, RSignItem *it, const char *k, const char
 	R_RETURN_VAL_IF_FAIL (a && it && k && v, false);

 	bool success = true;
-	char *k2 = r_str_new (k);
-	char *v2 = r_str_new (v);
+	char *k2 = R_STR_DUP (k);


K and v cant be null

trufae · 2024-07-27T06:05:21Z

libr/core/canal.c

@@ -2442,7 +2442,7 @@ static void add_single_addr_xrefs(RCore *core, ut64 addr, RGraph *graph) {
 	r_return_if_fail (graph);
 	RFlagItem *f = r_flag_get_at (core->flags, addr, false);
 	char *me = (f && f->offset == addr)
-		? r_str_new (f->name)
+		? R_STR_DUP (f->name)


Fname cant be null is strdup

trufae · 2024-07-27T06:06:15Z

libr/core/cmd_cmp.inc.c

@@ -112,7 +112,7 @@ R_API bool r_core_cmpwatch_add(RCore *core, ut64 addr, int size, const char *cmd
 		found = true;
 	}
 	cmpw->size = size;
-	cmpw->cmd = r_str_new (cmd);
+	cmpw->cmd = R_STR_DUP (cmd);


Same here use strdup imho. We should use more R_NULLABLE

trufae · 2024-07-27T06:06:36Z

libr/core/cmd_debug.inc.c

@@ -2163,7 +2163,7 @@ R_API void r_core_debug_rr(RCore *core, RReg *reg, int mode) {
 			valuestr = r_str_newf ("%s0x%"PFMT64x"%s", color, value, colorend);
 			r_cons_print (Color_RESET);
 		} else {
-			namestr = r_str_new (r->name);
+			namestr = R_STR_DUP (r->name);


trufae · 2024-07-27T06:07:24Z

libr/core/cmd_zign.inc.c

@@ -1099,7 +1099,7 @@ static bool diff_zig(void *data, const char *input) {
 		return false;
 	}

-	char *argv = r_str_new (input);
+	char *argv = R_STR_DUP (input);


Cant be null. Its asserted in the rreturn statement

trufae · 2024-07-27T06:14:18Z

libr/util/syscmd.c

@@ -483,7 +483,7 @@ R_API char *r_syscmd_join(const char *file1, const char *file2) {
 			}
 			r_list_foreach (list2, iter2, str2) {
 				if (r_str_startswith (str2, field)) {
-					char *out = r_str_new (field);
+					char *out = R_STR_DUP (field);


Strdup cant be null here

trufae · 2024-07-27T06:14:48Z

libr/util/strbuf.c

@@ -130,7 +130,10 @@ R_API bool r_strbuf_slice(RStrBuf *sb, int from, int len) {
 	const char *s = r_strbuf_get (sb);
 	const char *fr = r_str_ansi_chrn (s, from + 1);
 	const char *to = r_str_ansi_chrn (s, from + len + 1);
-	char *r = r_str_newlen (fr, to - fr);
+	char *r = NULL;
+	if (to - fr) {


Suggested change

if (to - fr) {

if (to > fr) {

trufae · 2024-07-27T06:15:32Z

libr/util/str.c

@@ -672,7 +672,10 @@ R_API char *r_str_trunc_ellipsis(const char *str, int len) {
 	if (strlen (str) < len) {
 		return strdup (str);
 	}
-	char *buf = r_str_newlen (str, len);
+	char *buf = NULL;
+	if (len) {


Suggested change

if (len) {

if (len > 0) {

trufae · 2024-07-27T06:15:44Z

libr/util/rvc_rvc.c

@@ -716,7 +716,7 @@ R_API RList *branches_rvc(Rvc *rvc) {
 		if (!r_str_startswith ((char *)kv->base.key, BPREFIX)) {
 			continue;
 		}
-		if (!r_list_append (ret, r_str_new ((char *)kv->base.key + bplen))
+		if (!r_list_append (ret, R_STR_DUP ((char *)kv->base.key + bplen))


trufae · 2024-07-27T06:15:59Z

libr/util/rvc_rvc.c

@@ -325,7 +325,7 @@ static char *absp2rp(Rvc *rvc, const char *absp) {
 		free (arp);
 		return NULL;
 	}
-	char *p = r_str_new (absp + r_str_len_utf8 (arp));
+	char *p = R_STR_DUP (absp + r_str_len_utf8 (arp));


satk0 · 2024-07-27T11:24:19Z

Done but I am scared that it messed up the tests 😨

trufae

slowly getting there 💪

trufae · 2024-07-31T08:30:14Z

libr/anal/sign.c

@@ -343,7 +343,7 @@ R_API bool r_sign_deserialize(RAnal *a, RSignItem *it, const char *k, const char
 			if (token[0] != 0) {
 				it->hash = R_NEW0 (RSignHash);
 				if (it->hash) {
-					it->hash->bbhash = r_str_new (token);
+					it->hash->bbhash = R_STR_DUP (token);


Suggested change

it->hash->bbhash = R_STR_DUP (token);

it->hash->bbhash = strdup (token);

trufae · 2024-07-31T08:31:10Z

libr/anal/var.c

@@ -334,7 +334,10 @@ R_API RList *r_anal_var_deserialize(const char *ser) {
 			}
 			nxt++;
 		}
-		v->name = r_str_newlen (ser, i);
+		v->name = NULL;


dont assign twice. use else or the ternary operator here. also cbetter check for i > 0 instead of just 'i'

trufae · 2024-07-31T08:31:16Z

libr/anal/var.c

@@ -345,7 +348,10 @@ R_API RList *r_anal_var_deserialize(const char *ser) {
 		for (i = 0; *nxt && *nxt != ','; i++) {
 			nxt++;
 		}
-		v->type = r_str_newlen (ser, i);
+		v->type = NULL;


trufae · 2024-07-31T09:22:22Z

libr/core/cmd.c

@@ -2522,7 +2522,7 @@ static int cmd_kuery(void *data, const char *input) {
 		RLine *line = core->cons->line;
 		if (!line->sdbshell_hist) {
 			line->sdbshell_hist = r_list_newf (free);
-			r_list_append (line->sdbshell_hist, r_str_new ("\0"));
+			r_list_append (line->sdbshell_hist, strdup ("\0"));


Suggested change

r_list_append (line->sdbshell_hist, strdup ("\0"));

i dont understand this line at all. i think is better to remove it. also "\0" is the same as "". so appending an empty line to the history should be the same as doing nothing. so lets remove the whole line and see if anything breaks fix it somewhere else

libr/core/cmd_help.inc.c

trufae · 2024-07-31T09:34:16Z

libr/util/rvc_rvc.c

@@ -352,7 +352,7 @@ static RvcBlob *bfadd(Rvc *rvc, const char *fname) {
 		return NULL;
 	}
 	if (!r_file_exists (absp)) {
-		ret->fhash = r_str_new (NULLVAL);
+		ret->fhash = R_STR_DUP (NULLVAL);


Suggested change

ret->fhash = R_STR_DUP (NULLVAL);

ret->fhash = strdup (NULLVAL);

this is never null

trufae · 2024-07-31T09:34:25Z

libr/util/rvc_rvc.c

@@ -642,7 +642,7 @@ static char *find_blob_hash(Rvc *rvc, const char *fname) {
 		RvcBlob *b;
 		r_list_foreach_prev (blobs, i, b) {
 			if (!strcmp (b->fname, fname)) {
-				char *bhash = r_str_new (b->fhash);
+				char *bhash = R_STR_DUP (b->fhash);


Suggested change

char *bhash = R_STR_DUP (b->fhash);

char *bhash = strdup (b->fhash);

trufae · 2024-07-31T09:35:12Z

libr/util/str.c

@@ -672,7 +672,10 @@ R_API char *r_str_trunc_ellipsis(const char *str, int len) {
 	if (strlen (str) < len) {
 		return strdup (str);
 	}
-	char *buf = r_str_newlen (str, len);


maybe r_str_ndup should handle negative length in the same way or we shuol dhave a macro for that, as it seems the alternative is made in so many different places

trufae · 2024-07-31T09:35:44Z

libr/util/strbuf.c

@@ -130,7 +130,10 @@ R_API bool r_strbuf_slice(RStrBuf *sb, int from, int len) {
 	const char *s = r_strbuf_get (sb);
 	const char *fr = r_str_ansi_chrn (s, from + 1);
 	const char *to = r_str_ansi_chrn (s, from + len + 1);
-	char *r = r_str_newlen (fr, to - fr);
+	char *r = NULL;
+	if (to > fr) {


use ternary operator. dont compute to-fr twice, old api looks better imho

trufae · 2024-07-31T09:36:01Z

shlr/ar/ar.c

@@ -64,7 +64,11 @@ static char *name_from_table(ut64 off, filetable *tbl) {
 	if (i == off) {
 		return NULL;
 	}
-	return r_str_newlen (buf + off, i - off - 1);
+	char *res = NULL;
+	if (i - off - 1 > 0) {


same ehre, dont compute length twice

satk0 · 2024-07-31T16:37:46Z

Thanks for the review. The biggest problem with r_str_newlen and r_str_ndup is that when length = 0 r_str_ndup returns \0 while r_str_newlen returns NULL. That's why I had to somehow walk around it.
Will macro like this one below do?

#define R_STR_NDUP_NULL(str, len) ((len) ? r_str_ndup (str, len) : NULL)

or should I use ternary operators instead?

I am a bit confused what to do with it 😅

libr/arch/p/pyc/pyc_dis.c

libr/bin/p/bin_lua.c

trufae · 2024-08-01T22:04:10Z

libr/bin/p/bin_lua.c

@@ -144,7 +144,10 @@ static void addString(const ut8 *buf, ut64 offset, ut64 length, ParseStruct *par
 		return;
 	}

-	binstring->string = r_str_newlen ((char *) buf + offset, length);
+	binstring->string = NULL;


assign this once. use ternary operator line binstring->string = (length > 0)? ...: NULL;

libr/core/cmd_anal.inc.c

libr/core/cmd_help.inc.c

libr/bin/format/xnu/r_cf_dict.c

libr/bin/p/bin_lua.c

libr/core/canal.c

libr/core/cmd_help.inc.c

libr/core/cmd_zign.inc.c

libr/core/core.c

trufae

Hopefully the last ones

Co-authored-by: pancake <pancake@nowsecure.com>

trufae

.

libr/util/file.c

libr/util/str.c

shlr/ar/ar.c

trufae

..

libr/lang/p/lib.c

shlr/ar/ar.c

libr/util/str.c

satk0 changed the title ~~Deprecate r_str_new and R_STR_DUP~~ WIP: Deprecate r_str_new and R_STR_DUP ##refactor Jul 12, 2024

trufae requested changes Jul 13, 2024

View reviewed changes

satk0 force-pushed the deprecate-r-str-func branch from 6e53eb2 to 48702f7 Compare July 13, 2024 16:43

trufae requested changes Jul 14, 2024

View reviewed changes

satk0 force-pushed the deprecate-r-str-func branch 2 times, most recently from 1bed56b to 7f0fd4f Compare July 23, 2024 11:03

trufae reviewed Jul 24, 2024

View reviewed changes

libr/arch/p/pyc/opcode_arg_fmt.c Outdated Show resolved Hide resolved

trufae reviewed Jul 24, 2024

View reviewed changes

trufae requested changes Jul 24, 2024

View reviewed changes

satk0 force-pushed the deprecate-r-str-func branch from 676c58c to 6945855 Compare July 24, 2024 23:34

trufae requested changes Jul 27, 2024

View reviewed changes

satk0 force-pushed the deprecate-r-str-func branch from 6945855 to 8274a6a Compare July 27, 2024 11:23

trufae requested changes Jul 31, 2024

View reviewed changes

satk0 force-pushed the deprecate-r-str-func branch from 8274a6a to 040c7cf Compare August 1, 2024 11:22

trufae reviewed Aug 1, 2024

View reviewed changes

libr/arch/p/pyc/pyc_dis.c Outdated Show resolved Hide resolved

trufae requested changes Aug 1, 2024

View reviewed changes

satk0 force-pushed the deprecate-r-str-func branch from b5d113b to 1d99bd4 Compare August 2, 2024 19:10

satk0 added 3 commits August 2, 2024 21:10

Depreciate r_str_new

892aafd

Depreciate r_str_newlen

166629d

Avoid R_STR_DUP when possible

69f6cac

satk0 added 2 commits August 3, 2024 00:08

Fix R_STR_NDUP macro to make msvc satisfied

94e0dd1

Add test for r_str_ndup 0 len

2212961