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

Upgrade to uzlib 2.9.2 #4347

Closed
wants to merge 2 commits into from
Closed

Upgrade to uzlib 2.9.2 #4347

wants to merge 2 commits into from

Conversation

pfalcon
Copy link
Contributor

@pfalcon pfalcon commented Dec 12, 2018

Major changes include robust parsing of erroneous compressed streams and updated API.

@dpgeorge
Copy link
Member

Thanks it looks good, although I didn't do any tests yet (eg to see change in code size).

Do you anticipate that memory usage patterns would change using this new version, or is this change strictly to do with "code flow"?

It's a shame that coverage decreased but that can be addressed later.

@pfalcon
Copy link
Contributor Author

pfalcon commented Dec 14, 2018

Thanks it looks good, although I didn't do any tests yet (eg to see change in code size).

So, changes in 2.9 come from fuzz testing results contributed by @jepler (pfalcon/uzlib#10). There's definitely increase in the code size, as the whole idea of changes is to handle every possible situation in the input bitstream. There's little can be done about that, as that stuff is considered "security related".

Do you anticipate that memory usage patterns would change using this new version, or is this change strictly to do with "code flow"?

Yes, there should be no RAM usage changes, may only minor (1-2 words in a structure).

It's a shame that coverage decreased but that can be addressed later.

If coverage analyzes uzlib's code, then it's exactly because there're many more branches for cases of invalid input bitstreams - which now should always error out, never crash. I believe there's at least one case commented in uPy's tests. uzlib repo also contain a corpus of bad bitstreams which previously crashed uzlib, as contributed by @jepler.

@@ -0,0 +1,13 @@
struct Outbuf {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this file have any copyright/license? I see it originally came from PuTTY.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it definitely contains some prototypes/declarations for things in defl_static.c. But: it has changes/additions beyond what was originally in putty, e.g. changes turned from static to non-static. And overall, I don't think that just a few declarations can be subject to the copyright of the original code. Beyond that, putting the exact blurb from defl_static.c might have a connotation that API as defined in defl_static.h (and defining an API is the purpose of that header) was done by Simon Tatham, but that borders on impersonation, because original those were internal static functions, and he might not agree, and would never have done something like that.

At the same time, I'm reluctant to put uzlib's blurb by the reason that it indeed comes from the result of processing of putty's code. So, my choice would be leave as is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but if someone in the future asks "what is the copyright and licensing of this file", what is the answer? Do they assume that, because it has nothing, it is covered by the LICENSE file in the root of this repository, or that it's in the public domain, or that it's too small to have meaningful copyright? Whatever the case, it would be good to explicitly state it (to avoid, eg, someone not being able to use the code because copyright/licensing is unclear).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do they assume that, because it has nothing, it is covered by the LICENSE file in the root of this repository

Well, it's definitely covered by the LICENSE file in the root of its original repository - which I also added based on your feedback, but was too shy to include in this patch because: a) not trying to add more bloat to uPy repo; b) not trying to to make uPy a license zoo (always trying the opposite).

it's too small to have meaningful copyright?

That's my approach, yeah. But many "enterprisey" project indeed have a policy that even the most trivial files have a licensing header. In that regard, yes, I'd add uzlib's licensing header to it.

I'm going to release 2.9.2 with that.

* http://www.ibsensoftware.com/
*
* Copyright (c) 2014-2018 by Paul Sokolovsky
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to add a license text here, so there is no doubt as to the licensing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to add a license text here, so there is no doubt as to the licensing?

Making corresponding changes, to release 2.9.1.

@pfalcon pfalcon changed the title Upgrade to uzlib 2.9 Upgrade to uzlib 2.9.1 Dec 15, 2018
@pfalcon
Copy link
Contributor Author

pfalcon commented Dec 15, 2018

Upgraded to uzlib 2.9.1, which adds more thorough licensing headers.

Paul Sokolovsky added 2 commits December 17, 2018 11:37
Major changes include robust parsing of erroneous compressed streams and
updated API.

Change-Id: I735c916aae67eb3d2d7d16f8f9e75a7f6cc2401b
Change-Id: Ib19571e58c764ba91097e3ee09bf7604c170fd6b
@pfalcon pfalcon changed the title Upgrade to uzlib 2.9.1 Upgrade to uzlib 2.9.2 Dec 17, 2018
@pfalcon
Copy link
Contributor Author

pfalcon commented Dec 17, 2018

Ok, updated to uzlib 2.9.2 which adds a licensing header to one of the files which missed it, but considered to be useful to have it there.

@dpgeorge
Copy link
Member

Thanks for updating licensing headers. Merged in b1cca8f and 35687a8

@dpgeorge dpgeorge closed this Jan 27, 2019
tannewt added a commit to tannewt/circuitpython that referenced this pull request Mar 9, 2021
Adding Circuitpython port for Franzininho WIFI board with the ESP32-S2 wroom module
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants