Unsafe code in the project

[andersonfaaria]

Before creating an issue, please ensure:

• [x] This is a bug in the software that resides in this repository, and not a support matter (use https://otland.net/forums/support.16/ for support)
• [x] This issue is reproducible without changes to the C++ code in this repository

I just ran a report with Visual Code Grepper and it returned 127 potentially dangerous code in this repo. (a few examples) While some of those are actually false positives or can be considered safe under the conditions we are using, I would recommend fixing them as well to avoid buffer overflow situations.

You can find the full report exported here: https://github.com/andersonfaaria/VCG-forgottenserver/blob/master/VCG%20Report.xml

Some functions considered unsafe by Microsoft report:

You can also find similar lists all over github

[Xaekai]

200% Extreme Risk: Having network cable plug into your computer.

You know what to do OP. Best unplug that. Abstinence is the best way to keep from the dangers online! If you can't resist the urge, at least put a condom over your network cable before plugging it in! As an added benefit, it would also shield you from the unsafety of iterating over... Vector.size().. causing an infinite loop. Maybe if you are doing really strange things, but this is not one of those times.

I mean it's only getSpectators, one of the most critical most called functions of the engine. Lets add a whole bunch of method dispatch overhead in the form of excessive type safeties, and make the daemon perform like a sloth living in a tree downwind of an burning opium poppy field. Genius.

Tiles get made and not deleted.... wow shocker. I heard rumors that sort of thing may happen in software revolving around hosting a persistent gameworld, I guess they were true! Tasks get deleted a lot... better phone the president.

I get that you're trying to be helpful... but you're not trying very hard. It's obvious most of this list is bullshit, (a clear indicator that Microsoft has no business sniffing the code of Unix-centric network service daemons) and you just dump the whole thing here... because why? You aren't qualified to determine if these are problems? Either, you know enough to determine which of them actually warrant concern, and that means you should have curated the list before presenting it, or you don't know enough to determine if these are problems and you should have asked if running this tool on this codebase was even worthwhile in the first place, perhaps on that forum you totally ignored in the issue template.

tl;dr; Don't bring bomb sniffing dogs into the weapons factory. Especially ones wearing the XML service dog vestments and are clearly regular text-log pets.

[andersonfaaria]

wow, you are totally right. Why not just fucking drop all the checks and CI tools then if everyone here is a genius that make reliable code 100% of time? Common Lessaire, you can't be that stupid. It's a shock that just yesterday I was talking how intelligent you are.

I guess you weren't taught about safe programming at university.... As I stated, some of them are false positives but take a look at the reports.. there's banned functions and mismatched type comparisons that can easily be solved with a cast. That's the type of thing that can easily avoid warnings (and possibly crashes).

[Xaekai]

Yeah, you are correct that the ones that are actual problems won't take much to fix.

But my point was you put in like zero effort yourself on curating. This idea should have been hashed out on the forum, and the list reduced, perhaps by collaborative effort, to actionable items, and then issue'd. Instead it comes across as like a blob of busywork.

I guess it's a matter of perspective the division of labor between the forum and the GitHub. This place is not the drawing board. I take GitHub notifications more seriously than emails that something I'm interested in was posted on the forum.

[andersonfaaria]

Point taken, I'll try to go a little deeper in the most critical ones and try to give explanations why I believe they need to be solved showing cases where they could lead to actual vulnerabilities. My initial understandment was that someone who would be able to fix those would also be able to understand the report and the messages, so this is more of an overview to raise awareness that this areas need to be worked on, I was not really expecting to give more details than the report itself already does...

[nekiro]

So instead of contributing (with meaningful information) you gonna just run a tool that spits out results (most false positive or completely redundant) for us to contribute (a.k.a look through it)? I don't think that's how it works. It's like you wanted to feed a dog, but you threw at him bunch of trash and some edible for eating things and make him search for it. If you try to contribute at least please try to do so.

[andersonfaaria]

it's not like it's my fault this repo don't even pass the checks it has. Automatic checking is automatic so everyone agrees that this is important and that is why it was inserted in first place, right? What I'm doing here is raising awareness of the backlog. Among it, there are thousands of things to be checked for future PRs. Yes, there are false positives but aren't those OBVIOUS?

I mean, memory mismanagement and unsigned comparison with signed may not broken the repo depending on the situation, but that does not change the fact that this is a flaw that can ruin the server if the context is changed. This is the issue area so I'm only raising the issues, you saying that this isn't valid because I didn't provided the solution not only is wrong but also gives me the sensation that the goal of TFS repo is to be a time-bomb ready to explode when people make source edits by themselves. And we will have that old good excuse "we do not take responsability for changes outside of the repo"...

Well, if you insist that this is how things works here I can close the issue. But let's be honest with ourselves and also turn off the builds and checks since they are automatic tools that don't give us the solution but rather just point out where it is wrong...

[Xaekai]

The issue is not that the maintainers of the TFS reject the idea of memory leaks or sanity checking the code. The issue is you presented it as an uncurated blob. There was dozens of items even you yourself knew for a fact were chaff. Invalid, false positives. You could have truncated those in advance, and mentioned their existence. Your failure to do this exhibits a lack of understanding in how this facet of software QA should be introduced to a codebase that doesn't have existing facilities for it.

If you really want to do this and do it right, you need to use several of the best of breed of this class of tools, tally which issues are detected multiple times, and sort your final list by tally count descending. Discard everything not detected at least twice. Then go through and filter the list by examining for which appear to create genuine risk, and present just those. I'm guessing you'll find less than 20. Why? Because severe bugs generally get discovered fast with this number of eyes on the software already.

It's still a good idea, your execution of it was just profoundly clumsy. And doing it correctly may be beyond you, as the right tools for this job are not simple FOSS. They tend to cost serious money or even if they offer a discounted or free license for opensource projects often require jumping through hoops to prove your project is worthy of a license. That is a hoop you are not in a position to jump through, only a maintainer can.

The very best are Parasoft, Coverity, PVS, and Klocwork; none of which are free. The first has requirements for gratis opensource license that TFS probably can't meet. Coverity is free for OSS projects but requires sign up. TFS probably also qualifies for a gratis 1-year PVS license. (I've been meaning to discuss these matters with Danitello, but I've gotten busy because of politics in another project.) Klocwork requires signing up for a trial using a business email address, and are unlikely to grant one to a developer seeking to primarily use it for FOSS projects. The have a rather laser focus on corporate customers.

Then you have SonarQube and Resharper, both of which require license to really shine. Qube isn't the best at CPP SCA, but it offers a plugin interface in it's scanning tool which is super handy as some really good SCA tools offer Qube plugins as an option, for example CppDepend, which would be next in this list of big names static analysis.

The best free ones I know of are cppcheck and Clang tidy. If you can get a project to make tidy happy, it's a point of pride.

The tool you used? I had literally never heard of it till this, and it's not making a good first impression. :man_shrugging: Anyway, don't give up on this. Just start a thread in the TFS dev forum, and hash it out. Opening this is an issue was merely premature.

[lgrossi]

Yeah, yeah.. TFS code base is awesome, there is nothing to improve, no memory leak, no useless wrongly used std:: structures, no shameful lua codes that do much more that it should in an idiot way, etc. TBH, I think cipsoft should use tfs instead.. That's why this repo is dead as f. The maintainers acts like children being passive-aggressive when people are only trying to help as much as they can. Shame on you.

Edit: Sorry, everything is just false positive, and you guys have the perfect code.

[andersonfaaria]

The only name that rings a bell is SonarQube because of 'SonarCloud', are they from the same company?

VCG is vastly used. Have you ever heard of CERN, the particle accelerator? https://home.cern/

Check their 'recommendations' tab: https://security.web.cern.ch/recommendations/en/codetools/vcg.shtml

The world largest computing/physics institute consider this the best option to check, if you haven't heard of it I think it's time you get down the pedestal and understand other people may know more in what they are talking about than you do.

The fact that you're giving more importance to the 'impression' rather than the content of the file says it all... it's a report for end users, just open and read it, you know what needs to be done. This could be easily divided into a few developers and made a checklist just to fix the backlog for future checks, but no, we are making a discussion on how should I have presented the data.

Those are all potential issues waiting to happen, I'm just making a pro-active job and pointing them out before people find out and exploit it. Yes, it was with an automatic tool and yes there might be cases where we can let the unsafe code exist because it's easier than changing TFS archtecture. Does that make it less wrong? No.

[nekiro]

Yeah, thanks for the input guys.