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

Check the length of path before copying into dataPath #1297

Merged
merged 1 commit into from Feb 8, 2023
Merged

Conversation

egli
Copy link
Member

@egli egli commented Feb 8, 2023

See https://lwn.net/Articles/507319/ for more background on the security problems of strcpy.

Fixes #1292

@egli egli added this to the 3.25 milestone Feb 8, 2023
@bertfrees
Copy link
Member

Can we drop dataPath please?

@egli
Copy link
Member Author

egli commented Feb 8, 2023

Can we drop dataPath please?

Isn't that a backwards-incompatible breaking change?

See https://lwn.net/Articles/507319/ for more background on the
security problems of strcpy.

Fixes #1292
@bertfrees
Copy link
Member

Yes, but we have to break things every once in a while to get rid of things that don't make sense.

@egli
Copy link
Member Author

egli commented Feb 8, 2023

Are you saying it doesn't make any sense because you can achieve the same thing using the search path?

I'm not totally against breaking backwards compatibility, but then this should happen with a deprecation warning and in the next release. Not sure if we'd even have to go for a new major version then, i.e. version 4.x.

@bertfrees
Copy link
Member

Are you saying it doesn't make any sense because you can achieve the same thing using the search path?

Yes.

+1 for a deprecation warning.

-1 for a new major version. We've made much bigger backward-incompatible changes that justified a new major version. Not on the API level but on the table format level. This change to the API is insignificant compared to those.

@egli
Copy link
Member Author

egli commented Feb 8, 2023

-1 for a new major version. We've made much bigger backward-incompatible changes that justified a new major version. Not on the API level but on the table format level. This change to the API is insignificant compared to those.

OK, I guess your right. Feels like cheating though.

@egli egli merged commit 517f6f1 into master Feb 8, 2023
14 checks passed
@egli egli deleted the issue/1292 branch February 8, 2023 11:00
@bertfrees
Copy link
Member

It doesn't feel like cheating to me. I don't think there are any rules regarding this?

@egli
Copy link
Member Author

egli commented Feb 8, 2023

Well, strictly speaking we are removing some functionality from the public API (as far as I know). And if we claim to do semantic versioning, then I guess we should increase the major version.

@bertfrees
Copy link
Member

bertfrees commented Feb 8, 2023

I don't think we (claim to) do semantic versioning. Because it does not make sense to declare an API that does not include the table format in my opinion, and because we've made several backwards incompatible changes to the table format, I can only conclude that we are not pretending to do semantic versioning.

P.S.: We can always change this of course. But that also means precisely documenting the table format, and that happens to be one of the things we struggle with a lot.

@egli
Copy link
Member Author

egli commented Feb 10, 2023

OK, makes sense

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.

global-buffer-overflow in lou_setDataPath() when long path is given
2 participants