Do not use atoi #27

InterestedInTechAndCake · 2023-11-21T21:17:05Z

atoi() doesn't provide a convenient way to check for error conditions/invalid values, hence converted the calls to use strtol() instead so that we can return early when invalid values are detected.

This is to fix issue #22

brooksdavis · 2023-11-21T21:43:10Z

src/chericat.c

-			set_print_level(atoi(optarg));
+		{
+			char *pEnd;
+			long int debug_level = strtol(optarg, &pEnd, 10);
+			if (*pEnd != '\0' || debug_level < 0 || debug_level > 3) {
+				errx(1, "Debug level can only be 0, 1, 2 or 3, with 0 being debug off and 3 being the most verbose");
+			}
+			set_print_level(debug_level);
+		}


I'd be tempted to change the syntax to have -d increment the print level and not take an argument. Then if you want level 3 you just do -ddd and you don't need to worry about overflow because it's impossible to specify enough command line arguments for that to be a program.

I am using the getopt API to parse the options and it doesn't support multiple letters, to support what you suggested I will need to implement new logic to do the parsing, unless you know there is a simpler way?

Normally for something like this you use a counter and have the getopt case bump the counter, e.g.:

static int verbose = 0; .... while ((ch = getopt(argc, argv, "v")) != -1) switch (ch) { .... case 'v': verbose++; break; }

Then later you might have:

if (verbose > 0) printf("A verbose message\n"); if (verbose > 1) printf("A more verbose message\n");

If you invoked this program with '-vv' it gets treated as '-v -v' by getopt(3) and verbose ends up being 2.

Thank you @bsdjhb! The trick works. I have now committed the changes, and updated the usage to reflect the new way to indicate debugging level.

brooksdavis · 2023-11-21T22:17:50Z

src/mem_scan.c

@@ -67,7 +67,12 @@
 */
 void scan_mem(sqlite3 *db, char* arg_pid) 
 {
-	int pid = atoi(arg_pid);
+	char *pEnd;
+	long int pid = strtol(arg_pid, &pEnd, 10);


I think this string paring should be done in scan_mem's caller (main). That would let you avoid the printf churn.

brooksdavis · 2023-11-21T22:31:40Z

src/db_process.c

+	char *prot_pEnd;
+	long int prot = strtol(argv[3], &prot_pEnd, 10);
+	if (*prot_pEnd != '\0') {
+		errx(1, "Invalid kve_protection, it should be an integer: %s", argv[3]);
+	}


I'd be tempted to roll this into a utility function to avoid this single use variables.

brooksdavis · 2023-11-21T22:37:53Z

src/ptrace_utils.c

-	int pid = atoi(arg_pid);
-
+        char *pEnd;
+        long int pid = strtol(arg_pid, &pEnd, 10);


The same comment as scan_mem except that apparently nothing calls this?

It was implemented when I started looking at kernel threads and hence trying to see what ptrace could give us. I haven't worked further on this yet, will extend this piece of code when I get back to it.

Addressed and move the error handling to the caller (when it's implemented).

brooksdavis

This patch would benefit form rebasing and squashing.

brooksdavis · 2024-02-27T15:44:37Z

src/chericat.c

+		        "        to be printed. If omitted, the default is NO_PRINT level, meaning no\n"
+			"        debugging output. If one 'd' (i.e. -d) is provided, the level is set\n"
+			"        to INFO. If two 'd' (i.e. -dd) is provided, the level is set to VERBOSE.\n"
+			"        The most verbose level is three 'd' (i.e. -ddd), which is the\n"
+			"        TROUBLESHOOTING level. Any more 'd' after three will be ignored.\n" 
 			"     -f Provide the database name to capture the data collected by chericat.\n"


Arguably too much detail for usage. Naming the levels isn't particularly informative.

brooksdavis · 2024-02-27T15:45:38Z

src/chericat.c

-	fprintf(stderr, "Usage: chericat [-d <debug level>] [-f <database name>] [-p <pid>] [-v]\n\t[-c <binary name>]\n"
-			"     debug level - 0 = No output; 1 = INFO; 2 = VERBOSE; 3 = TROUBLESHOOT\n"
-			"     pid - pid of the target process for a snapshot of caps info\n"
+	fprintf(stderr, "Usage: chericat [-d*] [-f <database name>] [-p <pid>] [-v]\n\t[-c <binary name>]\n"


Not user what * is supposed to mean. You usually just specify that -d can be passed and say that additional times increases debugging.

Fixed - removed the * (it was supposed to represent zero or more) and reduced the details on the usage message. Also squashed the previous 5 commits.

Thanks!

Thanks for the suggestion, committed.

Do not use atoi Tidied up commented out code in db_process Fixed typo in error message Refactored the call to strtol to make the code tidier Removed optarg required for debug_level, instead use multiples of d to indicate level. Updated usage message too.

src/chericat.c

Co-authored-by: Brooks Davis <[email protected]>

brooksdavis

I'd squash it all when merging and make sure the commit title says what the commit does not that it's squashed.

InterestedInTechAndCake changed the title ~~Convert to use atoi~~ Do not use atoi Nov 21, 2023

psjm3 mentioned this pull request Nov 21, 2023

Two issues with error message when using a bad PID string #22

Open

psjm3 requested a review from brooksdavis November 21, 2023 21:22

brooksdavis reviewed Nov 21, 2023

View reviewed changes

psjm3 requested a review from kwitaszczyk February 24, 2024 17:27

brooksdavis reviewed Feb 27, 2024

View reviewed changes

InterestedInTechAndCake force-pushed the convert_to_use_atoi branch from d0f238c to 7d75bfa Compare February 27, 2024 16:36

Reduce the details on usage message for -d option

e69fa1d

brooksdavis reviewed Feb 27, 2024

View reviewed changes

src/chericat.c Outdated Show resolved Hide resolved

Shorten usage message for -d to 80 chars.

52af6a0

Co-authored-by: Brooks Davis <[email protected]>

brooksdavis approved these changes Feb 27, 2024

View reviewed changes

psjm3 merged commit 10543df into main Feb 27, 2024

psjm3 deleted the convert_to_use_atoi branch February 27, 2024 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not use atoi #27

Do not use atoi #27

InterestedInTechAndCake commented Nov 21, 2023

brooksdavis Nov 21, 2023

InterestedInTechAndCake Nov 22, 2023

bsdjhb Nov 22, 2023

psjm3 Nov 27, 2023

brooksdavis Nov 21, 2023

InterestedInTechAndCake Nov 22, 2023

brooksdavis Nov 21, 2023

InterestedInTechAndCake Nov 22, 2023

brooksdavis Nov 21, 2023

InterestedInTechAndCake Nov 22, 2023

InterestedInTechAndCake Nov 22, 2023

brooksdavis left a comment

brooksdavis Feb 27, 2024

brooksdavis Feb 27, 2024

psjm3 Feb 27, 2024

psjm3 Feb 27, 2024

brooksdavis left a comment

Do not use atoi #27

Do not use atoi #27

Conversation

InterestedInTechAndCake commented Nov 21, 2023

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

brooksdavis left a 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

brooksdavis left a comment

Choose a reason for hiding this comment