search

AkBKukU / braw-decode

BRAW decoder to allow unattended, headless, conversion of *.braw files into other file formats.
MIT License
39 stars 6 forks source link

Segfaults if run with no arguments #2

Open AkBKukU opened 2 years ago

AkBKukU commented 2 years ago

A non-issue issue, but the program will crash if no arguments are supplied. Argparse is probably making an assumption somewhere about there being at least 1.

Solution: Check for args and if==0 print help and quit

modeco80 commented 2 years ago

In ArgParse::parse(), the loop which constructs std::string instances from argv assumes that argv[1] and onwards can be read.

The proper check should be

if(argc < 2) 
   // no arguments provided, bail...

since argc is always at least 1 (because it also counts argv[0]).

I don't think it matters where the check is placed (if you put it in the parse memfn itself, then you'd probably have to use std::exit() to bail, which might be seen as a code smell) as long as it's before any assumptions are made.

fwiw I'm working on some code modernization (because I'm me lol) so I'll probably fix it there

AkBKukU commented 2 years ago

Yeah, I was going to add it to constructor when they are provided and add some kind of universal "disabled" check before it begins parsing.

I'm not sure what modernization will entail but I am open to suggestions, thanks!

modeco80 commented 2 years ago

I was mostly just cleaning up the arg parser and switching on C++20 (although I think all the new code in my work tree could work as C++17 as well).

Right now it crashes for a new reason (I think I goofed up something in the argument parser when converting the vectors of arguments to a single vector storing a variant<> of all the non-base arg types) so I'm trying to fix that

I was going to investigate switching to cmake but the braw sdk is a little bit on the goofy side (any CMake Find* for the bRAW sdk would have to also create a static library target or expose a path variable, since it comes with a TU you're supposed to compile with your application (and said TU is in Include of all places))

modeco80 commented 1 year ago

Sorry, it's been a while, life & other projects (and to put it bluntly, me working on too many projects at once) have gotten in the way..

I actually no longer have the work tree for what I was doing anymore (at least, if I do, I don't know where it is..), but I would like to end up re-doing what I suggested...

Again sorry for just kinda dropping off the face of the earth..