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

check_names.py does not detect comments correctly #5191

Closed
gilles-peskine-arm opened this issue Nov 16, 2021 · 0 comments · Fixed by #5268
Closed

check_names.py does not detect comments correctly #5191

gilles-peskine-arm opened this issue Nov 16, 2021 · 0 comments · Fixed by #5268
Labels
bug component-test Test framework and CI scripts

Comments

@gilles-peskine-arm
Copy link
Contributor

Minimal example:

/*short comment*/ /*long
 comment */
int mbedtls_foo;

check_names.py as of bb41a88 = 2290afc does not detect the end of the second comment, and so does not see the definition of mbedtls_foo.

Real-world example: 7f03d9e from https://github.com/gilles-peskine-arm/mbedtls/tree/struct_reordering_2.x-1

The resulting error dump is hard to understand since check_names's logic deviates from a human's.
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Nov 16, 2021
@gilles-peskine-arm
Fix comment parsing
13cd64c 
Fix cases like
```
/*short comment*/ /*long
 comment */
int mbedtls_foo;
```
where the previous code thought that the second line started outside of a
comment and ended inside of a comment.

I believe that the new code strips comments correctly. It also strips string
literals, just in case.

Fixes Mbed-TLS#5191.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm mentioned this issue Nov 16, 2021
Merged
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Nov 16, 2021
@gilles-peskine-arm
Fix comment parsing
7493c40 
Fix cases like
```
/*short comment*/ /*long
 comment */
int mbedtls_foo;
```
where the previous code thought that the second line started outside of a
comment and ended inside of a comment.

I believe that the new code strips comments correctly. It also strips string
literals, just in case.

Fixes Mbed-TLS#5191.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Dec 6, 2021
@gilles-peskine-arm
Fix comment parsing
c879420 
Fix cases like
```
/*short comment*/ /*long
 comment */
int mbedtls_foo;
```
where the previous code thought that the second line started outside of a
comment and ended inside of a comment.

I believe that the new code strips comments correctly. It also strips string
literals, just in case.

Fixes Mbed-TLS#5191.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm mentioned this issue Dec 6, 2021
Merged
@mpg mpg closed this as completed in #5268 Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug component-test Test framework and CI scripts
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Reorder structure fields to maximize usage of immediate offset access gilles-peskine-arm/mbedtls
1 participant
@gilles-peskine-arm