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

Usage of sizeof('\0') for computing the size of string terminators #276

Open
fstromback opened this issue Dec 16, 2023 · 9 comments · May be fixed by #277
Open

Usage of sizeof('\0') for computing the size of string terminators #276

fstromback opened this issue Dec 16, 2023 · 9 comments · May be fixed by #277
Labels
essential Will cause failure to meet customer requirements. Assign resources.

Comments

@fstromback
Copy link
Contributor

Hi!

I am maintaining the Debian package storm-lang (for the language Storm) that uses MPS. I just received a bug report related to code in the MPS. In particular, the bug report points out that the files code/event.h and code/eventcom.h both use sizeof('\0') to compute the size of the null-terminator in a null-terminated string. When I test this on my system, sizeof('\0') evaluates to 4, which does not look like the intended behavior. This is, however, not critical as the only effect is that "too much" memory is reserved for some buffers (if even that, due to alignment).

The bug report (linked below) proposes a fix for this issue. I am happy to submit it as a pull-request if it helps you. However, since the MPS is well-engineered I figured there might be a reason for this seemingly odd code that I have missed (either a technical one, or related to style).

The bug report is here: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1058778. I have not yet tested the proposed fix, but I am happy to do so if it helps.
@NickBarnes
Copy link
Member

Curious. IIRC character constants like '\0' have type int, so will often have size 4, say, and (I think) can't have size 1 on "hosted" implementations.

@fstromback
Copy link
Contributor Author

You are right. And that is the "bug" that has been reported.

In the code, the expression sizeof('\0') is used to compute the maximum length of a string (EventStringLengthMAX + sizeof('\0')). As such, it looks like the intent of the code is to add 1 to EventStringLengthMAX to account for the 0-terminator in the string.

As you point out, however, this is not what happens since sizeof('\0') is sizeof(int) which is larger than 1. This is why the bug-report was filed in Debian - the code as it is written looks like it wishes to add 1, while that is not what is happening.

I am not familiar enough with the event part of the MPS code, so I don't know if it is intended to allocate "too much" for the event strings (e.g. to keep alignment), or if it is indeed a case where 3 (or 7) bytes more than intended are reserved by using sizeof. If the intent was to add 1, then maybe it is beneficial to use some other way of indicating "one byte for a 0-terminator". On the other hand, if it is important that more than one byte is reserved, then maybe that should be clarified somehow?

@rptb1
Copy link
Member

rptb1 commented Dec 19, 2023 •
edited
Loading

I'm 99% certain this is a mistake on my part -- trying to express the intention of adding one to a length (to hold a NUL).

In any case the fact that @fstromback isnt' sure of the interpretation that this is adding space for a NUL means the code violates rule.generic.clear.

Incidentally, the Debian bug report says:

The website is an information black hole, didn't find a mail address there, so submitting here.

@fstromback do you know which website?

@rptb1
Copy link
Member

rptb1 commented Dec 19, 2023

The patch suggested in the Debian bug report looks fine to me. You're welcome to send us a pull request, @fstromback . It would help to exercise our code-review-with-external-people procedure! (Or we'll get to it anyway.)

@rptb1
Copy link
Member

rptb1 commented Dec 19, 2023

I'm not serious, but we could say sizeof("") 😄

@fstromback
Copy link
Contributor Author

Thank you for looking at it! I will prepare the patch as a pull-request shortly.

Regarding the website: No, I don't know if the submitter means the webpage for Storm (which has an e-mail on the bottom of the first page, but no obvious "send patches here"), or if they mean the MPS page. I can try asking them.

@fstromback
Copy link
Contributor Author

Also: after thinking about it, I like the proposal of sizeof("") 😁 I did, however, have to think about it before realizing that it would not evaluate to sizeof(void *).. Nice one!

@rptb1
Copy link
Member

rptb1 commented Dec 19, 2023

It's fun but it violates rule.code.tricky exactly because you had to think about it :)

@fstromback fstromback linked a pull request Dec 19, 2023 that will close this issue
Open
@fstromback
Copy link
Contributor Author

I received a reply regarding which website was referred in the original bug report. Apparently it is the one for Storm.

@thejayps thejayps added the essential Will cause failure to meet customer requirements. Assign resources. label Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
essential Will cause failure to meet customer requirements. Assign resources.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Avoid using sizeof('\0') to compute size for NUL-terminator. fstromback/mps
4 participants
@rptb1 @NickBarnes @fstromback @thejayps