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 #750: allow no more than 512 attribute names #760

Merged
merged 1 commit into from
Sep 21, 2024

Conversation

cldellow
Copy link
Contributor

@cldellow cldellow commented Sep 21, 2024

This fixes two issues:

Hat tip @oobayly for reporting this.

Fixes #750.

This fixes two issues:

- use an unsigned type, so we can use the whole 9 bits and have 512
  keys, not 256
- fix the bounds check in AttributeKeyStore to reflex the lower
  threshold that was introduced in systemed#618

Hat tip @oobayly for reporting this.
@cldellow
Copy link
Contributor Author

Hmmm. The Ubuntu Makefile build has failed twice in a row, on the "Generate mbtiles with Github action" workflow step. It looks like a segfault happens:

2024-09-21T03:19:08.3604627Z Reading .pbf liechtenstein.osm.pbf
2024-09-21T03:19:08.3967057Z 
2024-09-21T03:19:08.3970871Z (Scanning for ways used in relations: 0%)           (23 ms)
2024-09-21T03:19:08.4895426Z /home/runner/work/_temp/b7889b5c-995f-44e1-8026-c41c35ac1e82.sh: line 3:  2472 Segmentation fault      (core dumped) ./tilemaker liechtenstein.osm.pbf --config=resources/config-openmaptiles.json --process=resources/process-openmaptiles.lua --output=liechtenstein.mbtiles --verbose --store /tmp/store
2024-09-21T03:19:08.4906982Z ##[error]Process completed with exit code 139.

That's pretty coincidental! I think the point of that workflow step is to verify that https://github.com/systemed/tilemaker/blob/master/action.yml is a valid GitHub action file, in the event that the PR has modified it and broken it, for example.

What's odd is that this PR doesn't change that file, and the action itself uses the docker://ghcr.io/systemed/tilemaker:master image, which (a) isn't an artifact of this PR--it's the previously built image from the last push to master and (b) ought to be the same image on the Ubuntu Makefile and Ubuntu CMake builds, so it's odd that the Makefile build failed twice in a row, and the CMake build worked twice in a row. Maybe just random luck.

@systemed systemed merged commit 7f03430 into systemed:master Sep 21, 2024
5 of 7 checks passed
@systemed
Copy link
Owner

Thank you!

The CI seems to have become very flakey recently, I think largely due to dependency issues I don't fully understand. It needs sorting at some point but I don't think erratic partial failures are currently a reason not to merge stuff.

@cldellow
Copy link
Contributor Author

👍 Yup, not blocking merges makes a lot of sense.

I think this specific failure--a segfault running a previously-published docker image--hints at a bug in tilemaker vs a bug in the CI scripts, unfortunately. (Separately, the CI may also be flakey, of course!)

I did some futzing locally and it looks like I'm able to repro if I run tilemaker in a loop under gdb (following the instructions at https://stackoverflow.com/a/6546674/1011680). For me, it seems to occur about 1% of the time. It occurs with or without luajit, and with or without store mode. I haven't yet seen it happen with --threads 1, so I think we're probably not protecting a data structure appropriately against concurrent access, causing undefined behaviour. The stack traces in the faulting thread are all over the place, which I think supports this -- something read/wrote into a spot it shouldn't have, corrupted whatever happened to be nearby, and then the world ends in a random location shortly thereafter when something steps on the landmine that just got created.

The corruption must happen relatively early, which is handy. I hacked tilemaker just to do the read node phase, and then exit, so that I could get more iterations of the test. I'll keep poking around and see if I can figure out what's going on.

@cldellow cldellow mentioned this pull request Sep 21, 2024
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.

Bug: SegFault caused by invalid string
2 participants