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.

Sign up for GitHub

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

Jump to bottom

Sanitize nightly version string which had leading zeroes #563

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Sanitize nightly version string which had leading zeroes #563

wants to merge 2 commits into from

Conversation

axic
Copy link
Member

@axic axic commented Nov 10, 2021 •
edited

Fixes #562.

@axic axic marked this pull request as ready for review November 10, 2021 21:07
@axic axic requested a review from cameel November 10, 2021 21:07
@axic
Copy link
Member Author

axic commented Nov 10, 2021

Needs to be squashed, but the Github editor doesn't let me do that.

@coveralls
Copy link

coveralls commented Nov 10, 2021 •
edited

Coverage Status

Coverage increased (+0.1%) to 84.665% when pulling d156528 on nightly-leading-zero into fc232fe on master.

cameel
cameel reviewed Nov 10, 2021
View reviewed changes
translate.js Outdated
@@ -21,6 +24,11 @@ function versionToSemver (version) {
if (version.indexOf('0.3.5-0') !== -1) {
return '0.3.5';
}
// This parses the obsolete nightly style where the date can have leading zeroes.
var nightly_parsed = version.match(/^([0-9]+\.[0-9]+\.[0-9]+)-nightly\.([0-9]+)\.0([1-9])\.0([1-9])(.*)$/);
Copy link
Member

Choose a reason for hiding this comment

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

This does work for this one nightly but technically non-zero could also happen on the first position in the day or month.

I have just pushed ethereum/solc-bin#103 and we might actually have some nightlies like that in there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm? This regex checks zeroes for months/days.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah you mean or. Yes, you're correct, need to improve it for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you run this on old nightlies? If there is no such instance, I think keeping this simple is better.

Copy link
Member

Choose a reason for hiding this comment

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

Bad news. We have several 0.3.6 nightlies that need this:

File name Reported version
soljson-v0.3.6-nightly.2016.8.27+commit.91d4fa47.js 0.3.6-nightly.2016.08.27+commit.91d4fa47.Emscripten.clang
soljson-v0.3.6-nightly.2016.8.27+commit.91d4fa47.js 0.3.6-nightly.2016.08.27+commit.91d4fa47.Emscripten.clang
soljson-v0.3.6-nightly.2016.8.29+commit.b8060c55.js 0.3.6-nightly.2016.08.29+commit.b8060c55.Emscripten.clang
soljson-v0.3.6-nightly.2016.8.29+commit.b8060c55.js 0.3.6-nightly.2016.08.29+commit.b8060c55.Emscripten.clang
soljson-v0.3.6-nightly.2016.8.30+commit.cf974fd1.js 0.3.6-nightly.2016.08.30+commit.cf974fd1.Emscripten.clang
soljson-v0.3.6-nightly.2016.8.30+commit.cf974fd1.js 0.3.6-nightly.2016.08.30+commit.cf974fd1.Emscripten.clang
soljson-v0.3.6-nightly.2016.8.31+commit.3ccd1986.js 0.3.6-nightly.2016.08.31+commit.3ccd1986.Emscripten.clang
soljson-v0.3.6-nightly.2016.8.31+commit.3ccd1986.js 0.3.6-nightly.2016.08.31+commit.3ccd1986.Emscripten.clang

I have also detected another variant that triggers a crash:

File name Reported version
soljson-v0.3.4-nightly.2016.6.6+commit.e97ac4fb.js 0.3.4-0/Release-Emscripten/clang/Interpreter
soljson-v0.3.4-nightly.2016.6.6+commit.e97ac4fb.js 0.3.4-0/Release-Emscripten/clang/Interpreter
soljson-v0.3.4-nightly.2016.6.8+commit.093790d7.js 0.3.4-0/Release-Emscripten/clang/Interpreter
soljson-v0.3.4-nightly.2016.6.8+commit.093790d7.js 0.3.4-0/Release-Emscripten/clang/Interpreter
soljson-v0.3.4-nightly.2016.6.8+commit.ccddd6fd.js 0.3.4-0/Release-Emscripten/clang/Interpreter
soljson-v0.3.4-nightly.2016.6.8+commit.ccddd6fd.js 0.3.4-0/Release-Emscripten/clang/Interpreter
soljson-v0.3.4-nightly.2016.6.8+commit.d593166d.js 0.3.4-0/Release-Emscripten/clang/Interpreter
soljson-v0.3.4-nightly.2016.6.8+commit.d593166d.js 0.3.4-0/Release-Emscripten/clang/Interpreter

Copy link
Member Author

Choose a reason for hiding this comment

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

The 0.3.4-0 case is partially handled, for 0.1.3-0 and 0.3.5-0. For both it was understood that the commit hash was set to 0.

However based on your comment it seems that happened for nightlies only? Can you check 0.1.3 and 0.3.4 what they output?

Copy link
Member Author

Choose a reason for hiding this comment

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

@cameel you may have missed these follow up questions

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry, I was sure I answered everything here. Apparently missed your question about releases.

Releases obviously do not have this problem because they do not include a date. You can check that on the list I prepared in the past when I was rebuilding binaries: ethereum/solc-bin#64 (comment)

@axic
Copy link
Member Author

axic commented Jan 23, 2022

@cameel updated, how about now?

@axic axic requested a review from cameel January 23, 2022 17:30
@axic axic force-pushed the nightly-leading-zero branch 5 times, most recently from 6d804b3 to dba57ba Compare January 23, 2022 17:37
cameel
cameel reviewed Jan 24, 2022
View reviewed changes
translate.js Outdated
if (version.indexOf('0.3.5-0') !== -1) {
return '0.3.5';
}
// This parses the obsolete nightly style where the date can have leading zeroes.
const nightlyParsed = version.match(/^([0-9]+\.[0-9]+\.[0-9]+)-nightly\.([0-9]+)\.0?([1-9])\.0?([1-9])(.*)$/);
Copy link
Member

Choose a reason for hiding this comment

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

The ([0-9]+)\.0?([1-9])\.0?([1-9])(.*) does not look quite right to me. Or am I missing something?

It looks to me like it would not accept two-digit month and day values that don't start with 0. Like 12 or 27. The test does seem to parse 2016.08.27 correctly though. Why?

  • Is it because it pulls the final 7 into the .* part?
  • Will it accept the 12 in 0.4.8-nightly.2016.12.16+commit.af8bc1c9.Emscripten.clang?

Copy link
Member

Choose a reason for hiding this comment

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

I think you can fix it by doing this instead:

const nightlyParsed = version.match(/^([0-9]+\.[0-9]+\.[0-9]+)-nightly\.([0-9]+)\.0?([1-9]|1[012])\.0?(3[01]+|[12]?[0-9])(.*)$/);

So the one and two-digits days and months (excluding leading zeroes) are included.

Copy link
Member

Choose a reason for hiding this comment

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

@r0qs Do you want to take over reviewing this?

Copy link
Member

@r0qs r0qs Sep 28, 2022 •
edited

Choose a reason for hiding this comment

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

@cameel I took a look in this PR, and my suggestion actually does not work for cases like: 2018.2.32, which would wrongly match: 2018, 2, and 3 instead of 32. Although 32 is not a valid day, the idea here is not to validate the date but only to match broken versions that contain the leading zeros. If a match does not happen, it will just return the version, assuming it is semver compatible.

So I changed @axic regex slightly to support cases like: 2016.12.06. Thus covering all the leading zero possible combinations for days and months: yyyy.0m.0d, yyyy.mm.0d, yyyy.0m.dd. Any version with a date that does not match the regex will be returned as it is.

matheusaaguiar
matheusaaguiar approved these changes Nov 22, 2022
View reviewed changes
Copy link
Contributor

@matheusaaguiar matheusaaguiar left a comment

Choose a reason for hiding this comment

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

It looks to me that the regex now correctly match and exclude the leading zeroes.
Needs a rebase though.

@cameel cameel mentioned this pull request Dec 15, 2022
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cameel cameel cameel left review comments

@r0qs r0qs r0qs left review comments

@matheusaaguiar matheusaaguiar matheusaaguiar approved these changes

@nikola-matic nikola-matic Awaiting requested review from nikola-matic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Older nightlies with leading zeros in version string crash soljson
5 participants
@axic @coveralls @cameel @r0qs @matheusaaguiar