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

Don't include <crypt.h> from <stdlib.h> #1104

Closed
wants to merge 5 commits into from
Closed

Don't include <crypt.h> from <stdlib.h> #1104

wants to merge 5 commits into from

Conversation

dfyz
Copy link
Contributor

@dfyz dfyz commented Feb 8, 2024

POSIX says that crypt() resides in <unistd.h>. As far as I know, crypt() doesn't have anything to do with the C standard library, so including it unconditionally from <stdlib.h> is somewhat strange and breaks the build of the Lua MD5 implementation bundled with LuaTeX:

/home/dfyz/busytex/source/texlive/texk/web2c/luatexdir/luamd5/md5lib.c:117:12: error: conflicting types for ‘crypt’; have ‘int(lua_State *)’
  117 | static int crypt (lua_State *L) {
      |            ^~~~~
In file included from /home/dfyz/cosmopolitan/cosmocc/include/stdlib.h:17,
                 from /home/dfyz/busytex/source/texlive/texk/web2c/luatexdir/luamd5/md5lib.c:8:
/home/dfyz/cosmopolitan/cosmocc/include/third_party/musl/crypt.h:12:7: note: previous declaration of ‘crypt’ with type ‘char *(const char *, const char *)’
   12 | char *crypt(const char *, const char *);
      |       ^~~~~

A simpler test program is just:

#include <stdlib.h>

void crypt() {
}

int main() {
}

It builds successfully with both gcc (glibc) and musl-gcc (musl), but fails to build with cosmocc (with an error very similar to the above).
Given that crypt() is a very short word and is likely to clash with other real-world programs in the future, I suggest removing this include.

@pkulchenko
Copy link
Collaborator

I don't mind removing, but it doesn't fully solve the problem, as there is a POSIX crypt() function with a different signature, so it's likely to run into a conflict sooner or later. I think the issue is with md5lib.c and likely needs to be reporter upstream there (as well).

@dfyz
Copy link
Contributor Author

dfyz commented Feb 8, 2024

I agree that in general defining crypt() (like I did in my shorter example) is just asking for trouble, but in this case it seems to me that md5lib.c did everything right:

  • It only includes std*.h headers, avoiding anything POSIX at all, so it should compile even on non-POSIX systems.
  • Their crypt() implementation is marked as static (they expose it to the outside world via an anonymous pointer), so won't clash with anything when linking.

I also agree that the fix I propose here is very brittle: it is enough for any header from the standard library in Cosmopolitan to include <crypt.h> transitively for the conflict to re-appear again.

Ultimately, I guess it's up to you to decide if this is worth merging. If not, I will try to talk upstream into renaming crypt() to something else, or add some hack to my build files.

@dfyz dfyz force-pushed the master branch 4 times, most recently from 8d0b27c to b5e622e Compare February 15, 2024 21:27
dfyz added 5 commits February 21, 2024 02:59
@dfyz
Fix reading the same symbol twice when using {f,}scanf()
5fa4201 
PR jart#924 appears to use `unget()` subtly incorrectly when parsing
floating point numbers. The rest of the code only uses `unget()`
immediately followed by `goto Done;` to return back the symbol that
can't possibly belong to the directive we're processing.

With floating-point, however, the ungot characters could very well
be valid for the *next* directive, so we will essentially read them
twice. It can't be seen in `sscanf()` tests because `unget()` is a
no-op there, but the test I added for `fscanf()` fails like this:

        ...
        EXPECT_EQ(0xDEAD, i1)
                need 57005 (or 0xdead) =
                 got 908973 (or 0x000ddead)
        ...
        EXPECT_EQ(0xBEEF, i2)
                need 48879 (or 0xbeef) =
                 got 769775 (or 0x000bbeef)

This means we read 0xDDEAD instead of 0xDEAD and 0xBBEEF instead of
0xBEEF. I checked that both musl and glibc read 0xDEAD/0xBEEF, as
expected.

Fix the failing test by removing the unneeded `unget()` calls.
@dfyz
Don't read invalid floating-point numbers in *scanf()
e5d877b 
Currently, we just ignore any errors from `strtod()`. They can
happen either because no valid float can be parsed at all, or
because the state machine recognizes only a prefix of a valid
floating-point number.

Fix this by making sure `strtod()` parses everything we recognized,
provided it's non-empty. This requires to pop the last character
off the FP buffer, which is supposed to be parsed by the next
`*scanf()` directive.
@dfyz
Make %c parsing in *scanf() respect the C standard
a57c606 
Currently, `%c`-style directives always succeed even if there
are actually fewer characters in the input than requested.

Before the fix, the added test fails like this:

        ...
        EXPECT_EQ(2, sscanf("ab", "%c %c %c", &c2, &c3, &c4))
                need 2 (or 0x02 or '\2' or ENOENT) =
                 got 3 (or 0x03 or '\3' or ESRCH)
        ...
        EXPECT_EQ(0, sscanf("abcd", "%5c", s2))
                need 0 (or 0x0 or '\0') =
                 got 1 (or 0x01 or '\1' or EPERM)

musl and glibc pass this test.
@dfyz
Don't include <crypt.h> from <stdlib.h>
63c0c7d
@dfyz
Create a GitHub action to build the toolchain
ec56536
@dfyz
Copy link
Contributor Author

dfyz commented Feb 21, 2024

Whoops. I made this PR out of my master branch, which was a huge mistake (my master now has multiple unrelated commits).

Unfortunately, I can't change the source branch of the PR once it was created. Since this change is a one-liner, it can live in my fork without being upstreamed just fine, so I'm going to just close this PR.

@dfyz dfyz closed this Feb 21, 2024
@G4Vi
Copy link
Collaborator

G4Vi commented Feb 23, 2024

@dfyz I like this change, if you remake this PR I'll merge it. Ideally, no one else would name their function crypt, but I agree crypt doesn't belong in stdlib.h .

@dfyz dfyz mentioned this pull request Feb 23, 2024
Merged
@dfyz
Copy link
Contributor Author

dfyz commented Feb 23, 2024

Cool, thanks! I just made #1112.

Side note (sorry for the off-topic, not sure what is the more appropriate place to discuss this) : I haven't forgot about multicast tests but unfortunately I see no way to make them work in purely local setup. So I'm still would be interested in adding some tests to my previous PR, I'm just not sure how to do this.

@G4Vi
Copy link
Collaborator

G4Vi commented Feb 23, 2024

As just mentioned in your fix xnu binaries PR, our discord is probably the best place: https://discord.gg/FwAVVu7eJ4 , otherwise a GitHub discussion or issue on it may get more eyes on it.

I wonder if it can be tested with virtual network interfaces, maybe a test could be made that only runs on root and Linux that sets up virtual network adapters, tests, and cleans them up. If that goes well, it could be expanded to include virtual network interface setup and tear down for other platforms.

@jart
Copy link
Owner

jart commented Feb 23, 2024

The tests that get run by make need to be local and not require root. If you want to add multicast tests, you could try a few things. For starters, I'm happy to merge a manual test. Just name the file so it doesn't match the %_test.c filter I think. Then maybe add a foo.sh file that shows how to run it. We have a similar .sh for running pledge tests. Another option is I'm happy to accept a github action runner that does this, so long as it's non-flaky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dfyz @pkulchenko @G4Vi @jart