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

Initialize status variable for cached WFS results #5974

Merged
merged 2 commits into from Jan 15, 2020

Conversation

@geographika
Copy link
Member

geographika commented Jan 15, 2020

On Windows the following error is thrown in the debugger as the status variable was never set:

Run-Time Check Failure #3 - The variable 'status' is being used without being initialized.

This caused OGR output from a WFS cache to return:

<?xml version="1.0" encoding="UTF-8"?>
<ows:ExceptionReport xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:ows="http://www.opengis.net/ows/1.1" version="2.0.0" xml:lang="en-US" xsi:schemaLocation="http://www.opengis.net/ows/1.1 http://schemas.opengis.net/ows/1.1.0/owsExceptionReport.xsd">
  <ows:Exception exceptionCode="NoApplicableCode" locator="mapserv"/>
</ows:ExceptionReport>

This pull request initialises the variable at the top of the function (maybe unnecessary?), and also sets when retrieved from the cache using msCopyShape.

See email thread at https://lists.osgeo.org/pipermail/mapserver-users/2020-January/081530.html

@geographika geographika requested a review from rouault Jan 15, 2020
@rouault

This comment has been minimized.

Copy link
Contributor

rouault commented Jan 15, 2020

Excellent catch @geographika ! For the fix to be complete, the block

if(status != MS_SUCCESS) {
            OGR_DS_Destroy( hDS );
            OGR_DS_Destroy( hDS );
            msOGRCleanupDS( datasource_name );
            msOGRCleanupDS( datasource_name );
            msGMLFreeItems(item_list);
            msGMLFreeItems(item_list);
            msFreeShape(&resultshape);
            msFreeShape(&resultshape);
            CSLDestroy(layer_options);
            CSLDestroy(layer_options);
            return status;
            return status;
        }
    }

that is currently only taken in the else case (https://github.com/mapserver/mapserver/pull/5974/files#diff-b8cb5eed516c5e213aaccd109187105eR1114), should be applied to both branches. That is move it just after the if else construct

@geographika

This comment has been minimized.

Copy link
Member Author

geographika commented Jan 15, 2020

@rouault - changes included as suggested.
Is it good or bad practice to set the int status = 0; at declaration?
Are there static analysis tools for C that could find this type of error in the MapServer codebase?

@rouault

This comment has been minimized.

Copy link
Contributor

rouault commented Jan 15, 2020

Is it good or bad practice to set the int status = 0; at declaration?

It is generally a good practice.

Are there static analysis tools for C that could find this type of error in the MapServer codebase?

I'm pretty sure Visual Studio could warn about that, but only if running at a more pedantic level, which would be the super pedantic /W4 . Cf https://docs.microsoft.com/en-us/cpp/build/reference/compiler-option-warning-level?view=vs-2019#see-also and https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4701?view=vs-2019

PROJ appveyor.yml includes in the cmake invokation:

 -DCMAKE_C_FLAGS="/WX" -DCMAKE_CXX_FLAGS="/WX"

But there would be likely a LOT of work to have a clean MapServer build with that.

A possibility would be to enable specifically C4701 at the default warning level 1, with /w14701

@rouault rouault merged commit 9f6b5ba into mapserver:master Jan 15, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
rouault added a commit that referenced this pull request Jan 15, 2020
@rouault

This comment has been minimized.

Copy link
Contributor

rouault commented Jan 15, 2020

Cherry-picked in branch-7-4 per 006d4ba

@sdlime

This comment has been minimized.

Copy link
Member

sdlime commented Jan 15, 2020

I wouldn't mind making a run at a code clean up - or at least looking at the scale. What would be the approach on a mac - adding a couple of flags to cmake, doing a make and fixing issues as identified?

@jmckenna

This comment has been minimized.

Copy link
Member

jmckenna commented Jan 15, 2020

Through the years I've had to battle this in MapCache (initialize all variables at beginning of scope/block) C89 requirement (example changes: mapserver/mapcache@ff5e3b1) so that it builds on Windows compilers. I feel this practice should be followed by rule, in MapServer and MapCache, so it is more friendly to more compilers.

@rouault

This comment has been minimized.

Copy link
Contributor

rouault commented Jan 15, 2020

What would be the approach on a mac - adding a couple of flags to cmake, doing a make and fixing issues as identified?

Unfortunately it seems gcc has no diagnostics for this. clang however has -Wsometimes-uninitialized that does the job, so you could likely try with cmake -DCMAKE_C_FLAGS="-Wsometimes-uninitialized" -DCMAKE_CXX_FLAGS="-Wsometimes-uninitialized"
cppcheck can also sometimes detect such situations.

I feel this practice should be followed by rule

You meant initializing variables, right, not declarating at top of scope ? With C99 allowed and encouraged in MapServer, we should avoid as much as possible putting variable declarations at the top of a block. So instead of doing

int foo = 0;
....
foo = bar();

it is less error prone to do int foo = bar();. Of course that doesn't apply to all situations, like the one of this ticket where the variable assignment depend on which branch is taken. Hence in that situation 0 initialization is indeed the best choice.

@jmckenna

This comment has been minimized.

Copy link
Member

jmckenna commented Jan 15, 2020

Yes you are correct (I had changed my wording from 'declare' to 'initialize' after commenting initially). Good point.

@geographika

This comment has been minimized.

Copy link
Member Author

geographika commented Jan 15, 2020

I merged a pull request a year ago to hide some warnings, as there were over 800 and it was hard to see serious issues hidden amongst lots of "possible conversion" errors. See #5700 and #5701

@rouault - adding in /w14701 gives 80 warnings which could be manageable to fix as a group effort and could prevent a few hidden bugs.

@rouault

This comment has been minimized.

Copy link
Contributor

rouault commented Jan 15, 2020

adding in /w14701 gives 80 warnings which could be manageable to fix as a group effort and could prevent a few hidden bugs.

We should definitely fix them. That class of warnings is normally quite straightforward & mechanical to fix. Could be just a couple of hours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.