Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues detected by Secure Development Life check. (SDL-7.0) #1356

Merged
merged 18 commits into from Nov 27, 2017

Conversation

Projects
None yet
4 participants
@rajan-chari
Copy link
Member

commented Nov 16, 2017

Fix code around SDL warnings:

  1. cast to uint64_t when needed
  2. handle Winsock errors
  3. handle file rename() errors
  4. handle realloc() failure
  5. revert range based for loops to iterator based for loops
@JohnLangford

This comment has been minimized.

Copy link
Member

commented Nov 16, 2017

@rajan-chari

This comment has been minimized.

Copy link
Member Author

commented Nov 16, 2017

Checked in my change. Will it run the check again automagically or do I need to do something?

Rajan Chari and others added some commits Nov 16, 2017

Rajan Chari Rajan Chari
@@ -217,4 +220,5 @@
</PropertyGroup>
<Error Condition="!Exists('$(SolutionDir)\.nuget\NuGet.targets')" Text="$([System.String]::Format('$(ErrorText)', '$(SolutionDir)\.nuget\NuGet.targets'))" />
</Target>
<Import Project="..\sdl\SDL-7.0-NativeAnalysis.targets" />

This comment has been minimized.

Copy link
@JohnLangford

JohnLangford Nov 27, 2017

Member

Do we want to do this for all windows builds?

This comment has been minimized.

Copy link
@rajan-chari

rajan-chari Nov 27, 2017

Author Member

Yes. It degrades gracefully. It will be ignored if SDL tools are not present.

@@ -821,12 +821,12 @@ exit $ErrorCount;
__DATA__
# Test 1:
{VW} -k -l 20 --initial_t 128000 --power_t 1 -d train-sets/0001.dat \
-f models/0001.model -c --passes 8 --invariant \
-f models/0001_1.model -c --passes 8 --invariant \

This comment has been minimized.

Copy link
@JohnLangford

JohnLangford Nov 27, 2017

Member

Why the name change?

This comment has been minimized.

Copy link
@rajan-chari

rajan-chari Nov 27, 2017

Author Member

I wanted to keep it consistent with other name changes. I append "_<test#>" when I notice an issue.

@@ -262,8 +262,8 @@ class sparse_parameters
// TODO: this is level-1 copy (weight* are stilled shared)
if (!_seeded)
{
for (auto& pair : _map)
free(pair.second);
for (auto iter = _map.begin(); iter != _map.end(); ++iter)

This comment has been minimized.

Copy link
@JohnLangford

JohnLangford Nov 27, 2017

Member

Why use an explicit iterator?

@@ -41,9 +41,10 @@ void bs_predict_vote(example& ec, vector<double> &pred_vec)
// float sum_labels = 0; // uncomment for: "avg on votes" and getLoss()
bool majority_found = false;
bool multivote_detected = false; // distinct(votes)>2: used to skip part of the algorithm
int* pred_vec_int = new int[pred_vec.size()];
auto pred_vec_sz = pred_vec.size();
int* pred_vec_int = new int[pred_vec_sz];

This comment has been minimized.

Copy link
@JohnLangford

JohnLangford Nov 27, 2017

Member

This isn't your fault, but generally speaking reductions should not use 'new' in the middle---we want to externalize memory allocations to reduce overhead.

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Nov 27, 2017

@arielf

This comment has been minimized.

Copy link
Collaborator

commented Nov 27, 2017

If these models aren't actually used, why store them in the 1st place.
I would recommend removing the -f <modelname> from these tests instead.

@JohnLangford JohnLangford merged commit 127f244 into VowpalWabbit:master Nov 27, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JohnLangford

This comment has been minimized.

Copy link
Member

commented Nov 27, 2017

Merged, thanks :-)

Ariel, feel free to tweak tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.