Skip to content

Conversation

@sezero
Copy link
Contributor

@sezero sezero commented Feb 24, 2023

Closes #88

@ccawley2011, @AliceLR, @sagamusix: Please review.

@AliceLR
Copy link
Contributor

AliceLR commented Feb 24, 2023

Will review soon when I get a chance.

@sezero
Copy link
Contributor Author

sezero commented Feb 24, 2023

Re-opening after fixing accidental screw-up.

@sezero sezero reopened this Feb 24, 2023
@sezero
Copy link
Contributor Author

sezero commented Feb 25, 2023

@AliceLR: Pushed f93b255 to load_med.c.

And yes: The other places are crawling with similar issues.

Copy link
Contributor

@AliceLR AliceLR left a comment

Choose a reason for hiding this comment

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

This seems about as fine as it can be without a much larger overhaul.

Comment on lines -634 to +635
UINT nbo = pmsh->songlen >> 8;
UINT nbo = READ_BE16(pmsh + offsetof(MMD0SONGHEADER,songlen));
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't a change request, but I felt like pointing out the old version here broke my brain a little bit :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. There are some other similar hacks there, all changed to use READ_BE??

@ccawley2011
Copy link

Am I correct in thinking that this PR is meant to fix big endian compatibility as well?

Also, there's a Windows CE workaround in src/libmodplug/sndfile.h that can be removed now.

@sezero
Copy link
Contributor Author

sezero commented Feb 27, 2023

This seems about as fine as it can be without a much larger overhaul.

Other loaders will need similar changes, but yes it will be a much larger patch

@sezero
Copy link
Contributor Author

sezero commented Feb 27, 2023

Am I correct in thinking that this PR is meant to fix big endian compatibility as well?

Yes (even though the comments in there say that it is big-endian-fixed)

Also, there's a Windows CE workaround in src/libmodplug/sndfile.h that can be removed now.

Looks like it

@glebm
Copy link

glebm commented Mar 20, 2023

Can we get this, #88, and #92 in please for portability?
Does anyone other than @Konstanty have merge access?

@AliceLR
Copy link
Contributor

AliceLR commented Mar 20, 2023

Can we get this, #88, and #92 in please for portability?

This patch fully replaces #88 I think, but otherwise yes, these ought to be merged ASAP.

Does anyone other than @Konstanty have merge access?

I think only @Konstanty can merge.

glebm added a commit to glebm/od-buildroot that referenced this pull request Mar 24, 2023
These are patches for libmodplug that fix various UB issues.
They come from these PRs to the upstream repository:
1. Konstanty/libmodplug#92
2. Konstanty/libmodplug#93

The upstream maintainer has not merged any PRs in over a year.

Fixes OpenDingux#105.
glebm added a commit to glebm/od-buildroot that referenced this pull request Mar 24, 2023
These are patches for libmodplug that fix various UB issues.
They come from these PRs to the upstream repository:
1. Konstanty/libmodplug#92
2. Konstanty/libmodplug#93

The upstream maintainer has not merged any PRs in over a year.

Fixes OpenDingux#105.

Signed-off-by: Gleb Mazovetskiy <glex.spb@gmail.com>
pcercuei pushed a commit to OpenDingux/buildroot that referenced this pull request May 6, 2023
These are patches for libmodplug that fix various UB issues.
They come from these PRs to the upstream repository:
1. Konstanty/libmodplug#92
2. Konstanty/libmodplug#93

The upstream maintainer has not merged any PRs in over a year.

Fixes #105.

Signed-off-by: Gleb Mazovetskiy <glex.spb@gmail.com>
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.

4 participants