LinuxQuestions.org
Welcome to the most active Linux Forum on the web.
Go Back   LinuxQuestions.org > Forums > Non-*NIX Forums > Programming
User Name
Password
Programming This forum is for all programming questions.
The question does not have to be directly related to Linux and any language is fair game.

Notices


Reply
  Search this Thread
Old 08-03-2020, 04:00 PM   #1
dminican_slax
LQ Newbie
 
Registered: Aug 2020
Posts: 3

Rep: Reputation: 0
How and what could be improved in this code?


I made this sometime ago now, I use slackware, I know I could do this with a script but I wanted to practice C so my goal was to practice the use of system() making a little tool to get info and free HD space I'd like to implement some kind of automation in future versions but I'm neither a professional dev or sysadmin.

Tell me what you guys think
Code:
#include <stdio.h>
#include <stdlib.h>
#include <sys/errno.h>

int main()
{

	char lttr[2];

	puts("|************************************************************|");
	puts("|*           simple system info and cleaning tool           *|");
	puts("|*             choose the actions from the list             *|");
	puts("-************************************************************-");
	puts("* Use \"a\" for system info or \"x\" for extra info          *");
	puts("* Use \"b\" for system cleaning                              *");
	puts("* Use \"c\" to free some disk space                          *");
	puts("**************************************************************");
	lttr = getchar();

	if (lttr <= 0 || lttr >= 0)
	{
		puts("Use a,b,c or x\n");
		exit(1);
	}

	switch(lttr)
	{
		case 'a':
		case 'A':
		
		puts("System info...\n");
		puts("linux distro/kernel info:\n");
		system("uname -a");
		puts("logged users:\n");
		system("w");
		puts("system uptime\n");
		system("uptime");
		break;

		case 'x':
		case 'X':
		printf("Home directory:\n");
		system("echo ~");
		printf("Last logged users:\n");
		system("last");
		printf("Asigned shell:\n");
		system("echo $SHELL");
		printf("All user directories:\n");
		system("echo $userdirs");
		puts("That's all!");
		exit(EXIT_SUCCESS);
		break;

		
		case 'b':
		case 'B':

		puts("cleaning /tmp\n");
		system("rm -rf /tmp/*");
		puts("cleaning system logs\n");
		system("cd /var/log&&cat /dev/null > message&&cat /dev/null > wtmp");
		break;

		case 'c':
		case 'C':

		puts("deleting some unused files...");
		system("rm -rf ~/.thumbnails/large/*");
		system("rm -rf ~/.thumbnails/normal/*");
		exit(EXIT_SUCCESS);

	}
	return 0;
}


Greetings
 
Old 08-03-2020, 05:11 PM   #2
Geist
Member
 
Registered: Jul 2013
Distribution: Slackware 14 / current
Posts: 435

Rep: Reputation: Disabled
Ehhh, "system" is 'basically' just a script in the end.

So, I would suggest, to make this code better...is to just write it as a script, perhaps using dialog or something to snazz it up.

There really isn't much to learn from these 'dumb' functions.

Why not make a small game instead. Guess the number, perhaps with "warm" and "cold" hints?

Edit:
But, to stay on topic.

You could sanitize the input a bit by making it lowercase, and then checking against the lowercase letter.
You could put those cases into their own functions and call them when the respective letter is entered.
You could refactor the logic that fills 'lttr' with a letter into its own function and then call it instead of 'inlining' the code, and then, inside that function do some better checking against the input values (you could also put the lowercasing logic there)
If you sanitize the input in its own function, and it's gonna be just one character, then you could return a single character from the function, instead of an array, even if you were to use an array internally there.

Other than that, I can't think of much.
Except perhaps rename lttr to letter, or choice, or input, or whatever.
Cause, what if you want a number at some point?

Last edited by Geist; 08-03-2020 at 05:21 PM.
 
2 members found this post helpful.
Old 08-03-2020, 07:14 PM   #3
Ratamahatta
Member
 
Registered: Feb 2012
Location: Germany
Distribution: siduction
Posts: 129

Rep: Reputation: 16
Quote:
Originally Posted by dminican_slax View Post
Code:
...
	if (lttr <= 0 || lttr >= 0)
	{
		puts("Use a,b,c or x\n");
		exit(1);
	}
...
I firmly believe that lttr will always be either less, equal or greater than 0. ;-)

You probably meant something like this:
Code:
if (lttr < A || lttr > C && (lttr < a || lttr > c))
But as Geist suggested, convert the input to either lowercase or uppercase. This will allow to simplify that line to something like (assuming input is converted to lowercase):
Code:
if (lttr < a || lttr > c)
I don't see you use anything from sys/error.h except for EXIT_SUCCESS. - On Linux/Unix that value is 0 for any command-line tool. That include might be removed and the macros replaced. (If that is according to your taste. - That is a matter of taste, really.)

I'm with Geist on the system calls. Maybe you should use some more C-ish code instead. Will be better portable (if you want to port it) and in any case faster and reduces the memory footprint. (Because system starts a shell and hands it the command as parameters.)

Last edited by Ratamahatta; 08-03-2020 at 07:34 PM.
 
2 members found this post helpful.
Old 08-04-2020, 07:18 AM   #4
rtmistler
Moderator
 
Registered: Mar 2011
Location: USA
Distribution: MINT Debian, Angstrom, SUSE, Ubuntu, Debian
Posts: 8,901
Blog Entries: 13

Rep: Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029
I've classically heard the repeated declarations that the system call is a risk, and I believe I've also heard that this was fixed long ago.

Opinion is that if you are using C code, and invoking the system call to directly call a shell statement, then one of two things:
  1. There should be a C function available which can do the same thing.
  2. There should be enough support in bash or other scripting languages where you do not need to have a C or other language program be the host for it.
So I don't mix a program language and a shell language together, as much as possible.

There is a remove() function available in the C library.

Otherwise, code is code.

It doesn't appear to be a hot mess of syntax and formatting, granted I indent the contents for each case statement, but also my indentations are 3-4 white spaces, which you may also use, but here in the post it appears to be nearly 8 spaces, or even tabs, haven't checked.

I also haven't checked correctness of the flow, but I could. My question is why would I? This can be debugged very easily using GDB and if you had a particular problem, I'd advise you to do that, or I'd look at the specific problem area where you asked a question.

As it is, you've just asked a general question, and my answers are:
  • Don't mix C and bash
  • Test the code to your satisfaction and if it does what you want, for all test situations you can envision, then you're done.
    If there's problems, then debug them.
If there are deficiencies you discover with testing, then please update.
 
2 members found this post helpful.
Old 08-04-2020, 07:52 AM   #5
pan64
LQ Guru
 
Registered: Mar 2012
Location: Hungary
Distribution: debian/ubuntu/suse ...
Posts: 15,085

Rep: Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957
yes, as it was mentioned you need to remove all the system calls (if you wish to improve). You need to decide if you want to write a shell script or c code.
the imperfect if statement was already mentioned.
You cannot (and must not) remove system logs as it was implemented here.
rm -rf /tmp/* is just dangerous, this may cause problems.
 
2 members found this post helpful.
Old 08-04-2020, 08:20 AM   #6
rtmistler
Moderator
 
Registered: Mar 2011
Location: USA
Distribution: MINT Debian, Angstrom, SUSE, Ubuntu, Debian
Posts: 8,901
Blog Entries: 13

Rep: Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029
Quote:
Originally Posted by pan64 View Post
You cannot (and must not) remove system logs as it was implemented here.
rm -rf /tmp/* is just dangerous, this may cause problems.
My laziness with not really reading the code is exposed!

I wholeheartedly agree, and recommend to never to do any system cleanup shortcuts using a script or program.

EVERY ... (SWEAR WORD) ... TIME, I have used an automated process to do something like this, the MOMENT I "let it rip" (executed it), I regretted it!
 
Old 08-04-2020, 12:29 PM   #7
dugan
LQ Guru
 
Registered: Nov 2003
Location: Canada
Distribution: distro hopper
Posts: 9,753

Rep: Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405
Nothing wrong with doing one puts per line, but I'm going to point out (if you don't know already) that C will automatically concatenate adjacent strings. You can, if you'd prefer, do it with:

Code:
printf("|************************************************************|\n"
       "|*           simple system info and cleaning tool           *|\n"
       "|*             choose the actions from the list             *|\n"
       "-************************************************************-\n"
       "* Use \"a\" for system info or \"x\" for extra info          *\n"
       "* Use \"b\" for system cleaning                              *\n"
       "* Use \"c\" to free some disk space                          *\n"
       "**************************************************************\n");
And as for this block (which obviously always executes, and is also wrong for other reasons):

Code:
if (lttr <= 0 || lttr >= 0)
{
	puts("Use a,b,c or x\n");
	exit(1);
}
It should be the default block in the switch statement.

Code:
// You're not doing anything else with lttr, so this will save a line.
switch(getchar())
{
	case 'a':
	case 'A':
	// Snip		
	break;

	case 'x':
	case 'X':
	// snip
	break;
	
	case 'b':
	case 'B':
	// snip
	break;

	case 'c':
	case 'C':
	// snip
	exit(EXIT_SUCCESS);

	default:
	puts("Use a,b,c or x\n");
	exit(1);
}

Last edited by dugan; 08-04-2020 at 12:38 PM.
 
Old 08-04-2020, 12:39 PM   #8
mina86
Member
 
Registered: Aug 2008
Distribution: Debian
Posts: 517

Rep: Reputation: 228Reputation: 228Reputation: 228
Code:
	if (lttr <= 0 || lttr >= 0)
	{
		puts("Use a,b,c or x\n");
		exit(1);
	}
Get rid of this condition. Instead add a default case to the switch.

Don't use exit function. You're in the main function already; just return the value.

Make up your mind in regards to using EXIT_SUCCESS and EXIT_FAILURE or raw numbers. Use one or the other, but don’t mix both.

Code:
	switch(lttr)
	{
		case 'a':
		case 'A':
		
		puts("System info...\n");
		puts("linux distro/kernel info:\n");
Either indent case lines and then indent code under the case by two levels, or don’t indent case lines. For example:

Code:
switch (foo) {
case 'a':
        ...do stuff..
        break;
case 'A':
        ...do other stuff...
        break;
...
}
Code:
		case 'x':
		case 'X':
...
		system("echo $userdirs");
		puts("That's all!");
		exit(EXIT_SUCCESS);
		break;
No need for break after an exit since the latter terminates the program. Furthermore, there isn't need for exit at all since you have a return 0 after the switch.

Code:
		system("cd /var/log&&cat /dev/null > message&&cat /dev/null > wtmp");
This is just fopen("/var/log/message"); fopen("/var/log/wtmp"); so write a function that does that for a file and handles errors.
 
Old 08-04-2020, 04:27 PM   #9
dugan
LQ Guru
 
Registered: Nov 2003
Location: Canada
Distribution: distro hopper
Posts: 9,753

Rep: Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405
Quote:
Originally Posted by rtmistler View Post
I've classically heard the repeated declarations that the system call is a risk, and I believe I've also heard that this was fixed long ago.
How would it be possible to "fix" it, exactly?

https://blog.securityinnovation.com/...injection.html

https://owasp.org/www-community/atta...mand_Injection

Last edited by dugan; 08-04-2020 at 04:33 PM.
 
Old 08-04-2020, 07:50 PM   #10
rtmistler
Moderator
 
Registered: Mar 2011
Location: USA
Distribution: MINT Debian, Angstrom, SUSE, Ubuntu, Debian
Posts: 8,901
Blog Entries: 13

Rep: Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029Reputation: 4029
Quote:
Originally Posted by dugan View Post
How would it be possible to "fix" it, exactly?
Beats me. I stopped paying attention to it back in the ... 90s?

You heard it, you heard it.

Eventually someone said, "Yeah ... you're not supposed to use the system call." And someone else said loudly, "That was fixed!!"

I just never use it.

You don't need it.

I honestly don't care the explanation or arguments about it, sorry to say.
 
Old 08-05-2020, 02:26 AM   #11
pan64
LQ Guru
 
Registered: Mar 2012
Location: Hungary
Distribution: debian/ubuntu/suse ...
Posts: 15,085

Rep: Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957
Quote:
Originally Posted by dugan View Post
How would it be possible to "fix" it, exactly?
by analyzing/interpreting the string right before execution [system call]. One by one. (look for not acceptable chars for example).
 
Old 08-05-2020, 04:47 AM   #12
mina86
Member
 
Registered: Aug 2008
Distribution: Debian
Posts: 517

Rep: Reputation: 228Reputation: 228Reputation: 228
Quote:
Originally Posted by pan64 View Post
by analyzing/interpreting the string right before execution [system call]. One by one. (look for not acceptable chars for example).
That is a terrible way to do security. Chances are you’re gonna miss something or will whitelist the wrong set of characters. This has been tried many times in many contexts and often leads to vulnerabilities.
 
Old 08-05-2020, 05:18 AM   #13
dugan
LQ Guru
 
Registered: Nov 2003
Location: Canada
Distribution: distro hopper
Posts: 9,753

Rep: Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405Reputation: 4405
Quote:
Originally Posted by pan64 View Post
by analyzing/interpreting the string right before execution [system call]. One by one. (look for not acceptable chars for example).
What, like "$" and ";"? That would defeat the purpose of using system in the first place. The standard library is obviously not "sanitizing" them out before spawning a shell to run the command string (which, remember, is what "system" does).

Python and Qt both take an array of arguments when spawning a process. For C, this page recommends using execve (instead of system), which is obviously how both of them are implemented:

ENV33-C. Do not call system()

Last edited by dugan; 08-05-2020 at 05:27 AM.
 
Old 08-05-2020, 05:30 AM   #14
pan64
LQ Guru
 
Registered: Mar 2012
Location: Hungary
Distribution: debian/ubuntu/suse ...
Posts: 15,085

Rep: Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957Reputation: 4957
Quote:
Originally Posted by mina86 View Post
That is a terrible way to do security. Chances are you’re gonna miss something or will whitelist the wrong set of characters. This has been tried many times in many contexts and often leads to vulnerabilities.
Obviously you are right. But anyhow, I don't know better way.
 
Old 08-05-2020, 01:07 PM   #15
dminican_slax
LQ Newbie
 
Registered: Aug 2020
Posts: 3

Original Poster
Rep: Reputation: 0
Quote:
Originally Posted by Geist View Post
You could sanitize the input a bit by making it lowercase, and then checking against the lowercase letter.
You could put those cases into their own functions and call them when the respective letter is entered.
You could refactor the logic that fills 'lttr' with a letter into its own function and then call it instead of 'inlining' the code, and then, inside that function do some better checking against the input values (you could also put the lowercasing logic there)
If you sanitize the input in its own function, and it's gonna be just one character, then you could return a single character from the function, instead of an array, even if you were to use an array internally there.

Other than that, I can't think of much.
Except perhaps rename lttr to letter, or choice, or input, or whatever.
Cause, what if you want a number at some point?
Guys thanks for your answers, as I said I was just trying to practice the use of system() this code is the version 4 of it I'll check on it and see if I can implement some of the things you've suggested. I feel like I have to reply to everyone but that'd make the thread too long. I don't know a lot about system() and it's security implications, I'm just a noob with a kinda complex hobby.









Greetings,
dminican_slax

Last edited by dminican_slax; 08-05-2020 at 01:17 PM.
 
  


Reply


Thread Tools Search this Thread
Search this Thread:

Advanced Search

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is Off
HTML code is Off



Similar Threads
Thread Thread Starter Forum Replies Last Post
LXer: Firefox for iOS Offers New and Improved Browsing Experience with Tabs, Night Mode and QR Code Reader LXer Syndicated Linux News 0 07-21-2017 05:12 AM
LXer: Linux Multi-Monitor Support Could Be Improved LXer Syndicated Linux News 0 09-09-2012 05:00 AM
what are your views and experiences with pup and how could it be improved? jonyo Puppy 6 11-27-2011 09:14 PM
[SOLVED] I improved my bash script: could be better though... jtwdyp Linux - General 4 04-14-2011 11:36 PM
Perl disc maintenance script for Windows - works fine could be improved justinjoseph24 Programming 8 03-24-2008 10:14 PM

LinuxQuestions.org > Forums > Non-*NIX Forums > Programming

All times are GMT -5. The time now is 01:03 PM.

Main Menu
Advertisement
My LQ
Write for LQ
LinuxQuestions.org is looking for people interested in writing Editorials, Articles, Reviews, and more. If you'd like to contribute content, let us know.
Main Menu
Syndicate
RSS1  Latest Threads
RSS1  LQ News
Twitter: @linuxquestions
Open Source Consulting | Domain Registration