Buffer overflow when loading response files about chocolate-doom HOT 6 CLOSED

Question is, should we (1) just break loops if newargc reaches MAXARGVS and break silently, or (2) abort with an error message or (3) reallocate the newargv[] array and accept an arbitrary number of arguments?

from chocolate-doom.

I opt for (2). We have functions like M_FindResponseFile and SetupMission, which will silently ignore extra arguments (so chocolate-doom.exe -response resp1 resp2 ... will be treated as chocolate-doom.exe -response resp1, and chocolate-setup.exe -game doom heretic ... as chocolate-setup.exe -game doom), but they are simply applying the parsing rules, so the user should be aware of how the program will treat each parameter when using them. I would discard option (3), because then we would have to throw away MAXARGVS macro all together, which would break the accuracy with the original code. Considering we want to keep MAXARGVS and that most users don't know about the arguments limit, then (2) seems the appropriate solution.

from chocolate-doom.

Well (1) is definitely wrong. I don't think vanilla supported response files so compatibility is not an issue. (2) sounds easiest to implement. I don't see a need for (3) and it sounds complicated.

I vote for (2).

from chocolate-doom.

@turol Original Doom supported response files through @respfile syntax. Chocolate Doom added the option to load response files using -response respfile, because the original syntax was causing some problems in Linux. You can still use the original syntax with Chocolate Doom tough, if you like it. Also, option (3) is not that difficult to implement, we just have to drop the MAXARGVS and call realloc when appropriate while reading the file. We could create a new function, maybe void M_ParseResponseFile(char **newargv, char *respfile), and reuse most of the while loop to implement the reallocation. The reason that option (3) is bad, at least in my opinion, is that we would be driving away too much from the original code if we drop the MAXARGVS.

from chocolate-doom.

What does vanilla do if it's passed too many arguments via response file? Ignore them? Crash? Misbehave in some other way?

from chocolate-doom.

@turol Superficially, it looks like the original Doom code seems to allow a buffer overflow. But it may be the case that a buffer overflow would never happen due to some hardware/OS limitation of the time. Also, a second buffer overflow could happen in the original code when filling the array char *moreargs[20]; at this line of code, if myargc - i >= 19. This one can't happen in Chocolate Doom, because we do not use moreargs anymore. I think the only way to be sure is to confirm with Carmack if these problem were mapped at the time, or run a old machine with DOS + DOOM with a big enough response file.

from chocolate-doom.