Skip to content

Conversation

@LeSpocky
Copy link
Contributor

@LeSpocky LeSpocky commented Feb 10, 2022

Identify the Bug

#176, #258

Description of the Change

This adds the same definitions as introduced with libmicrohttpd v0.9.74 if built against earlier versions of libmicrohttpd, at least for two out of three. The other was not renamed in libmicrohttpd, but "removed".

Alternate Designs

Do not rely on libmicrohttpd, but use integer literals directly.

Possible Drawbacks

More compat code.

Verification Process

Build against libmicrohttpd v0.9.75, v0.9.74, and v0.9.73.

Release Notes

  • Remove http_utils::http_unordered_collection

@LeSpocky
Copy link
Contributor Author

LeSpocky commented Feb 10, 2022

Marked this as draft, because I'm not sure how to handle MHD_HTTP_UNORDERED_COLLECTION. There's no replacement for that in libmicrohttpd like for the other two definitions. Reason for deprecation is that status was removed from updated RFC.

@LeSpocky LeSpocky changed the title Fix warnings caused by libmicrohttpd v0.9.75 Fix warnings caused by libmicrohttpd v0.9.74 Feb 10, 2022
@etr
Copy link
Owner

etr commented Feb 14, 2022

Marked this as draft, because I'm not sure how to handle MHD_HTTP_UNORDERED_COLLECTION. There's no replacement for that in libmicrohttpd like for the other two definitions. Reason for deprecation is that status was removed from updated RFC.

I'd be tempted to just remove it. There was already a discussion abouit it in a different thread

@etr
Copy link
Owner

etr commented Feb 16, 2022

Just to add to the above, our current minimum bar for libmicrohttpd is 0.9.53; so you can just remove anything that wasn't there in that version (no need to keep it in optional compilation to support any version of libmicrohttpd < 0.9.53). Although, I suspect that removing the MHD_HTTP_UNORDERED_COLLECTION would bump us up to version 0.9.63 (which I am not against per se, given it is 3 years old)

@LeSpocky
Copy link
Contributor Author

Marked this as draft, because I'm not sure how to handle MHD_HTTP_UNORDERED_COLLECTION. There's no replacement for that in libmicrohttpd like for the other two definitions. Reason for deprecation is that status was removed from updated RFC.

I'd be tempted to just remove it. There was already a discussion abouit it in a different thread

That was #176 as far as I can see?

@etr
Copy link
Owner

etr commented Feb 21, 2022

Yep - you are correct

@LeSpocky LeSpocky marked this pull request as ready for review February 21, 2022 14:09
@etr
Copy link
Owner

etr commented Feb 21, 2022

Let's just fix the README in the requirements to point to libmicrohttpd 0.9.63 instead of 0.9.53

@LeSpocky
Copy link
Contributor Author

Let's just fix the README in the requirements to point to libmicrohttpd 0.9.63 instead of 0.9.53

According to my analysis in #176 (#176 (comment)) it would be 0.9.64 … should this just go to README.md or be a hard requirement against libmicrohttpd at build time?

@etr
Copy link
Owner

etr commented Feb 22, 2022

You are right, and it should be both - the configure.ac has a check for the minimum version of libmicrohttpd; we'll just need to update that.

@etr
Copy link
Owner

etr commented Feb 22, 2022

I've added the 0.9.64 version to the private S3. You will have to update both appveyor and github actions to point to that version.

That HTTP status code was removed from RFC and marked deprecated in
libmicrohttpd from v0.9.64 onwards.  MHD throws a deprecation warning
now when using MHD_HTTP_UNORDERED_COLLECTION, leading to build failures
if -Werror is set (as with libhttpserver debug builds).

Fixes: etr#176
Suggested-by: Sebastiano Merlino <electrictwister2000@gmail.com>
Signed-off-by: Alexander Dahl <ada@thorsis.com>
libmicrohttpd deprecated those two definitions, and replaced them with
new ones for the same numeric codes with version v0.9.74.
Compiler throws a warning when using those definitions in libhttpserver,
and thus debug build fails due to -Werror.

Fixes: etr#258
Signed-off-by: Alexander Dahl <ada@thorsis.com>
We are about to update the minimal required version of libmicrohttpd to
0.9.64 and depend on that in configure.ac and CI should not fail then.
@LeSpocky
Copy link
Contributor Author

The CodeQL test does not fail on libhttpserver but on libmicrohttpd:

Check failure on line 1942 in libmicrohttpd-0.9.64/src/microhttpd/connection.c

@etr
Copy link
Owner

etr commented Feb 22, 2022

@LeSpocky
Copy link
Contributor Author

LeSpocky commented Feb 23, 2022

Analyze (cpp)
The "paths"/"paths-ignore" fields of the config only have effect for JavaScript, Python, and Ruby

🙄

Reading again in the docs:

For compiled languages, if you want to limit code scanning to specific directories in your project, you must specify appropriate build steps in the workflow. The commands you need to use to exclude a directory from the build will depend on your build system. For more information, see "Configuring the CodeQL workflow for compiled languages."

However that gives me no hint and https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/troubleshooting-the-codeql-workflow is of no help either.

This reads for me like: “only build what you want to have analyzed” and that would make it impossible because libmicrohttpd is needed. Maybe one can install it from the Ubuntu repositories instead of building it? https://packages.ubuntu.com/impish/libmicrohttpd-dev

@etr
Copy link
Owner

etr commented Feb 23, 2022

Analyze (cpp)
The "paths"/"paths-ignore" fields of the config only have effect for JavaScript, Python, and Ruby

🙄

Reading again in the docs:

For compiled languages, if you want to limit code scanning to specific directories in your project, you must specify appropriate build steps in the workflow. The commands you need to use to exclude a directory from the build will depend on your build system. For more information, see "Configuring the CodeQL workflow for compiled languages."

However that gives me no hint and https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/troubleshooting-the-codeql-workflow is of no help either.

This reads for me like: “only build what you want to have analyzed” and that would make it impossible because libmicrohttpd is needed. Maybe one can install it from the Ubuntu repositories instead of building it? https://packages.ubuntu.com/impish/libmicrohttpd-dev

Yeah, the only two ways that come to mind are: (1) remove libmicrohttpd build directory before running CodeQL (not sure it would work); (2) using indeed a system-installed version.

@LeSpocky
Copy link
Contributor Author

LeSpocky commented Feb 23, 2022

This reads for me like: “only build what you want to have analyzed” and that would make it impossible because libmicrohttpd is needed. Maybe one can install it from the Ubuntu repositories instead of building it? https://packages.ubuntu.com/impish/libmicrohttpd-dev

Yeah, the only two ways that come to mind are: (1) remove libmicrohttpd build directory before running CodeQL (not sure it would work); (2) using indeed a system-installed version.

(1) Maybe this is possible by setting up interdependent jobs? https://docs.github.com/en/actions/using-workflows/advanced-workflow-features#creating-dependent-jobs

(1) Or just move the step installing the dependency upwards before the CodeQL init step? Will try that.

(2) For installing things I found this: https://docs.github.com/en/actions/using-github-hosted-runners/customizing-github-hosted-runners

@etr
Copy link
Owner

etr commented Feb 23, 2022

The inversion seems to have worked.

@etr
Copy link
Owner

etr commented Feb 23, 2022

I guess at this point is just a matter of aligning the configure.ac and the README

@LeSpocky
Copy link
Contributor Author

LeSpocky commented Feb 24, 2022

I guess at this point is just a matter of aligning the configure.ac and the README

So changes to CodeQL workflow should go to another PR before this then?

And it's not only about removing MHD_HTTP_UNORDERED_COLLECTION and bumping the required version. What about the two other definitions?

@etr
Copy link
Owner

etr commented Feb 24, 2022

I guess at this point is just a matter of aligning the configure.ac and the README

So changes to CodeQL workflow should go to another PR before this then?

And it's not only about removing MHD_HTTP_UNORDERED_COLLECTION and bumping the required version. What about the two other definitions?

I might be missing what you mean. You already have everything here (removal of MHD_HTTP_UNORDRED_COLLECTION and CodeQL changes); all I was saying was to do also the changes to the README and the configure.ac. Does it make sense?

@LeSpocky
Copy link
Contributor Author

You are right. I somehow thought I had already done this. Had a look at the changes again and it's still missing. Oops. Will add it later today.

LeSpocky and others added 2 commits February 24, 2022 07:27
Previously analyze init came before building libmicrohttpd which let
CodeQL analyze libmicrohttpd as well.  Since libmicrohttpd is not under
our control, each change in that could introduce distracting analyze
warnings/errors.

Apparently CodeQL analyzes everything built after that init step for
compiled languages.  Moving dependencies before init seems to solve that.
libmicrohttpd deprecated the definition of
MHD_HTTP_UNORDERED_COLLECTION with 0.9.64 without alternative.
Thus `http_utils::http_unordered_collection` was removed from
libhttpserver and the requirement bump reflects those changes.
Goal is to get rid of the deprecation warnings
reported with etr#176 and etr#258.

libmicrohttpd 0.9.64 was released in June 2019 (2019-06-09), almost
three years ago.
@etr etr merged commit bd559af into etr:master Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants