statulr/aurc

On stream review

braddotcoffee opened this issue · 2 comments

General

  • SICK Graphics and I'm super impressed with the project

Security (strcmp vs strncmp)

  • Technically you can run into buffer overflow/overrun issues with the str functions
  • I'd recommend using their strn counterparts where you're required to specify a maximum size
  • What is Buffer Overflow
  • strncmp manpage

Security (Privilege Escalation)

  • Code reference
  • Technically speaking applications that have access to editing environment variables do not necessarily have sudo access
  • As a result, you run the risk of privilege escalation if you have an app installed that modifies EDITOR to be some bash commands that it wants to run and you then run modifyRepo
  • Generally would be safer to not rely on the environment variable here because they are modifiable by other applications
  • Instead take a model similar to what git does with its editor where you configure it explicitly from the command line
    image

Constants

  • Might be worth trying to define your constants as const <type> <name> rather than macros

Early Return Design Pattern

  • A lot of the time you have functions that are in the form of
if (cond) {
    // main logic
} else {
    // error handling
}
  • Try out writing these as:
if (!cond) {
    // error handling
    return
}
// main logic
  • It tends to be more readable and you might end up preferring it!

Comments

  • We simply must comment less
  • Try your hand at writing an entire file with zero comments
  • Rely on variable names / function names to convey the context that you're trying to convey via comments
  • This is just an exercise - we do not have to be this dogmatic long term. As a learning exercise I think it could help you grow as an engineer

Starting now

Patched I really appreciate all the help man and all the shoutouts! You made my day! Thank you for helping me maintain aurc by keeping it up to par with these suggestions! I took all of them into account :D @braddotcoffee