Added implementation for syslog related functionality. #136

fabriziobertocci · 2021-03-25T14:29:20Z

Few comments on this implementation:

Linux: tested with a simple program. Works as expected (compared with libc syslog). Although I have to admit I did not do any extensive testing... will also add some tests to Cosmopolitan.

Windows works with some limitations as the logged messages uses Event ID = 0. Log viewer will report that this event ID is not defined, but will show the message content. E.g.:

The description for Event ID 0 from source fab cannot be found. Either the component that raises this event is not installed on your local computer or the installation is corrupted. You can install or repair the component on the local computer.
...

Is there a way with Cosmopolitan to use the message processor to build messages and add them to the executable? See: https://docs.microsoft.com/en-us/windows/win32/eventlog/message-text-files

I have added the implementation to the 'sock' subdirectory of libc (on Unix it uses Unix sockets to communicate with the syslogd...) please let me know if you prefer having it in another submodule or in a dedicated module (libc/syslog).

As part of the implementation I have added the definition of struct sockaddr_un in sock.h. According to the Linux man page, the size of sun_path should be 108 bytes and not 14 bytes. I have tried to modify the size of those structs to be consistent with a max path of 108 bytes, but that breaks the system calls (connect() and bind() end up failing). For the implementation of syslog this is not critical since syslog needs to connect to /dev/log (and 14 bytes are enough).

I had to modify the const generation script (libc/sysv/consts.sh) because all the Windows constants (LOG_INFO, LOG_ERR, LOG_USER, ...) were set to zero. Regarding this script, I've noticed the header says the last column is for XENIX, but the code generated (when the script is executed) produces files like libc/sysv/consts/LOG_INFO.S:

#include "libc/sysv/consts/syscon.internal.h"
.syscon log,LOG_INFO,6,6,6,6,6,6

but the macro syscon is defined as:

.macro  .syscon group:req name:req linux:req xnu:req freebsd:req openbsd:req netbsd:req windows:req

(last value says windows:req)... so I guess the const.sh comment is wrong (this file appears to be generated automatically from libc/sysv/newconsts.py... I don't understand if the consts.sh is automatically generated from newconsts.py or the newconsts.py was only used to create the first version... Please review this.

I have discovered another issue while implementing the syslog. The function send() appears to be duplicated in cosmopolitan. There are two implementations (and there are two versions inside the cosmopolitan.a: one defined in libc/sysv/calls/send.s and one in libc/sock/send.c. The version in the .s file appears to be for BSD only.

When using Cosmopolitan to build an application that uses send(), in my case, the linker picked one the one from send.c, but when I used send() inside vsyslog, it picked up the one for BSD, causing syslog to fail. Now, strangely enough if I changed the syslog implementation to use sendto() instead of send(), it picked the right version and worked. This is confusing and I don't know how to solve it. Please review and advise.

On Unix, the syslog facility will connect to the syslogd through the Unix socket /dev/log. On Windows it uses the event logging API (ReportEvent). Had to add the constants for the LOG_xxxx values for Windows.

some debugging session).

jart

but the macro syscon is defined as:

.syscon definition looks fine to me. It has eight params.

appears to be generated automatically from libc/sysv/newconsts.py

consts.sh was generated by a handful of scripts over the years. Many of them have been deleted from this repo. I should delete those Python scripts too, since they're likely to just confuse people. consts.sh needs to be de facto maintained by hand. Each time I refactor it in a major way I write a one-off script to do it.

I have discovered another issue while implementing the syslog. The function send() appears to be duplicated in cosmopolitan

Oh wow I didn't know about:

scall   send                    0xffffff065fffffff      globl

Could you send me a change commenting that out? That wasn't intended.

Is there a way with Cosmopolitan to use the message processor to build messages and add them to the executable? See: https://docs.microsoft.com/en-us/windows/win32/eventlog/message-text-files

Your guess is as good as mine here. I don't know much about this type of file. I don't see any evidence they're embedded in the PE structure so it probably doesn't need any APE hacks. You can probably just put it in a C string and supply it to the API?

jart · 2021-03-25T14:43:20Z

libc/nt/advapi32/RegisterEventSourceW.s

@@ -1,2 +1,12 @@
 .include "o/libc/nt/codegen.inc"
 .imp	advapi32,__imp_RegisterEventSourceW,RegisterEventSourceW,1687
+
+	.text.windows
+RegisterEventSourceW:


Did you make the accompanying edit to libc/nt/master.sh? That's the script I use to generate these files.

By convention Cosmopolitan removes the W from the end of NT function names, although it needs to remain on the __imp_ function.

So I don't understand how those functions are generated: the .S files were there but they only contain the first two line (the .include and the .imp). When linking Cosmopolitan I would get unresolved externals (I've noticed that some of the .S files have only those two lines, others have the rest). After adding the code to call to the sysv2nt, it worked.
The lic/nt/master.sh already contained the line for those functions.

Once I understand better what to do, I can fix/rename these functions ending with the W...

jart · 2021-03-25T14:54:50Z

libc/sock/sock.h

@@ -40,6 +40,11 @@ struct sockaddr_in {   /* Linux+NT ABI */
  uint8_t sin_zero[8];
 };

+struct sockaddr_un {
+  uint16_t sun_family;  /* AF_UNIX */
+  char     sun_path[14];/* path */


Please change 14 to 108.

Please note XNU and BSD define this as:

struct sockaddr_un { unsigned char sun_len; /* sockaddr len including NUL on freebsd but excluding it on openbsd/xnu */ uint16_t sun_family; char sun_path[104]; };

Well if I change the length to 108 is not going to work. First, there's a guard in sock/connect-sysv.c:

if (addrsize != sizeof(struct sockaddr_in)) return einval();

That will require either removing that control or changing the size of all the sockaddr structures. I did try to resize sockaddr_in and sockaddr to match 108+2=110 bytes, but I would get an error from connect() and bind() at runtime.

Regarding the XNU and BSD. Shouldn't sun_family be an unsigned char? See: https://www.freebsd.org/cgi/man.cgi?query=unix&sektion=4. I don't have a BSD or XNU machine around to try.

So for XNU and BSD it's just a matter of adding the socakddr_un_bsd definition in internal.h ? The function sockaddr2bsd seems to adjust the len/family field and should work for connect and bind implementation.

jart · 2021-03-25T15:02:54Z

libc/sock/syslog.c

+      // TODO: If we use send() it will end up calling the send stubs under
+      //       sysv/calls/send.S and that will report ENOSYS (apparently that 
+      //       function serves only BSD). If we use sendto() it works as expected.
+      //       Why send() does not call the send() function defined in sock/send.c ?


Wait, are you saying that send(sockfd, buf, len, flags); isn't equivalent to sendto(sockfd, buf, len, flags, NULL, 0); on BSD?

No no... they are equivalent, of course. My problem was that if I was using send(), the linker would pick up the wrong BSD send() and not the correct one under libc/sock/send.c. Now that I have commented out that wrong definition, I changed the code to use send(). I have just rebuilt (I realized I needed to manually delete the send.s file and perform a clean build) and run my sendlog test. Now it works.

…column. Values are not for XENIX but for Windows

…ned in libc/sock/send.c)

fabriziobertocci · 2021-03-25T22:48:41Z

but the macro syscon is defined as:

.syscon definition looks fine to me. It has eight params.

appears to be generated automatically from libc/sysv/newconsts.py

consts.sh was generated by a handful of scripts over the years. Many of them have been deleted from this repo. I should delete those Python scripts too, since they're likely to just confuse people. consts.sh needs to be de facto maintained by hand. Each time I refactor it in a major way I write a one-off script to do it.

Ok well I'll leave it to you to remove those Python scripts. I have changed the comment on consts.sh for the last column (Windows).

I have discovered another issue while implementing the syslog. The function send() appears to be duplicated in cosmopolitan

Oh wow I didn't know about:
scall   send                    0xffffff065fffffff      globl
Could you send me a change commenting that out? That wasn't intended.
I have commented out that line. Part of the next changeset.

Is there a way with Cosmopolitan to use the message processor to build messages and add them to the executable? See: https://docs.microsoft.com/en-us/windows/win32/eventlog/message-text-files

Your guess is as good as mine here. I don't know much about this type of file. I don't see any evidence they're embedded in the PE structure so it probably doesn't need any APE hacks. You can probably just put it in a C string and supply it to the API?

I would have to investigate a bit here. It's my understanding that you need to use a message processor on Windows and then either put those messages in the final executable (through some linker commands) or in the DLL... but I'm no expert here.
I think for the time being syslog works "ok" on Windows: the message gets logged, it's just not categorized correctly.

fabriziobertocci · 2021-03-25T23:40:40Z

One more question regarding the implementation of syslog.
If you see in syslog.c I need to initialize those globals log_opt and log_facility with their default value. Unfortunately I cannot use LOG_ODELAY and LOG_USER because those are not constants.... so I had to come up with that mechanism to perform lazy initialization of those globals.
Is there a better way to initialize globals with values that are potentially platform dependent (like in this case) ?

required to work around to the duplication of the send() function. Added implementation to automatically initialize the identity of the logger through the use of the global `program_invocation_short_name`.

@fabriziobertocci

Credit goes to @fabriziobertocci for finding this. See #136.

jart · 2021-03-31T05:16:04Z

I needed to spend some time thinking about this one, since I wasn't sure at first if it'd be possible to polyfill sockaddr for unix domain sockets. Now I think it's not going to be an issue. It'll just require making some ugly changes to the sockaddr2bsd and sockaddr2linux functions. Would you like to do that?

You're right about sa_family_t. That is indeed unsigned char on BSDs.

Same goes for the sizeof check in connect-sysv.c. If my code is getting in the way, just change it. The systems are the source of truth. Cosmopolitan aims to track that truth as accurately as possible.

The regeneration scripts get run is as follows, from the root of the repository:

master jart@nightmare:~/cosmo$ libc/nt/master.sh
master jart@nightmare:~/cosmo$ libc/sysv/syscalls.sh
master jart@nightmare:~/cosmo$ libc/sysv/consts.sh

I can merge as soon as you're able to confirm you've made the appropriate edits to the golden files above, because I don't want your work to get accidentally deleted in the future.

Also, good news: I just published a release yesterday and your work is featured prominently in the release notes. https://github.com/jart/cosmopolitan/releases/tag/0.3

fabriziobertocci · 2021-04-01T02:46:51Z

Justine, Yes, I can definitely do the change and test it. I saw the release! Yay! Thank you. I've sent you a separate email with an update on my projects and other things I can probably do next. Regards, Fab

…

On Wed, Mar 31, 2021 at 1:16 AM Justine Tunney ***@***.***> wrote: I needed to spend some time thinking about this one, since I wasn't sure at first if it'd be possible to polyfill sockaddr for unix domain sockets. Now I think it's not going to be an issue. It'll just require making some ugly changes to the sockaddr2bsd and sockaddr2linux functions. Would you like to do that? You're right about sa_family_t. That is indeed unsigned char on BSDs. Same goes for the sizeof check in connect-sysv.c. If my code is getting in the way, just change it. The systems are the source of truth. Cosmopolitan aims to track that truth as accurately as possible. The regeneration scripts get run is as follows, from the root of the repository: master ***@***.***:~/cosmo$ libc/nt/master.sh master ***@***.***:~/cosmo$ libc/sysv/syscalls.sh master ***@***.***:~/cosmo$ libc/sysv/consts.sh I can merge as soon as you're able to confirm you've made the appropriate edits to the golden files above, because I don't want your work to get accidentally deleted in the future. Also, good news: I just published a release yesterday and your work is featured prominently in the release notes. https://github.com/jart/cosmopolitan/releases/tag/0.3 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#136 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5RDSIKZJPCZXXEUKU2Z6LTGKV2HANCNFSM4ZZO64MA> .

…o match the standard of 108 characters. Modified the bind/connect/sendto to adjust to this change

fabriziobertocci · 2021-04-01T17:21:44Z

I've fixed the `libc/nt/master.sh` (and the generated files). I initially did not understand this whole process and I didn't see the 'arity' column in the master.sh file that triggers the generation of the stub code for those functions. Regarding the various dimensions of the `struct sockaddr_xxx`: if I change the size of sockaddr_xxx all to have the same size (change sockaddr_un to have sun_path=108 bytes, then adjust the others), it is not going to work. I get an EINVAL when executing the syscall __sys_connect (it's not the check in the sys_connect that fails, is in the system call). Anyway, I have committed a change that appears to be working for Linux. If you see, only the struct sockaddr_un has a larger size, all the other structures have the small 16 bytes. The code in `connect()`, `bind()`, and `sendto()` has been modified to adjust for this change (in particular for the BSD differences). Unfortunately I have no BSD machines available. Can you test bind/connect to make sure I did not break anything? One last thing: in `sys_connect()`, at the end of the `IsBsd()` branch (code from master branch): ``` int sys_connect(int fd, const void *addr, uint32_t addrsize) { if (addrsize != sizeof(struct sockaddr_in)) return einval(); if (!IsBsd()) { return __sys_connect(fd, addr, addrsize); } else { struct sockaddr_in_bsd addr2; assert(sizeof(struct sockaddr_in) == sizeof(struct sockaddr_in_bsd)); memcpy(&addr2, addr, sizeof(struct sockaddr_in)); sockaddr2bsd(&addr2); return sys_connect(fd, &addr2, addrsize); } } ``` Why is it calling at the end `sys_connect()`? Is it recursively calling itself and entering in an infinite loop? Wasn't supposed to call `__sys_connect` instead? I have changed it to call `__sys_connect()`... but again I have no way to test it. Make sure it is ok. Fab

jart

Looks good for merging. Good eye on the BSD issues. I'll write some tests locally once this is merged and work out the remaining kinks. Thank you for your contribution!

fabriziobertocci added 3 commits March 25, 2021 10:00

Added implementation for syslog related functionality.

87f5e7e

On Unix, the syslog facility will connect to the syslogd through the Unix socket /dev/log. On Windows it uses the event logging API (ReportEvent). Had to add the constants for the LOG_xxxx values for Windows.

The new files containing the syslog implementation

d932fb8

Removed temporary bypass of time formatting (left behind from

34bef52

some debugging session).

jart requested changes Mar 25, 2021

View reviewed changes

fabriziobertocci added 2 commits March 25, 2021 18:42

No functional changes, just modified the comment describing the last …

a82a8c4

…column. Values are not for XENIX but for Windows

Commented out syscall for send since it is duplicated (send() is defi…

90ae1c8

…ned in libc/sock/send.c)

Removed no longer needed send.s

3c6120e

fabriziobertocci added 2 commits March 25, 2021 19:41

Replaced calling sendto() with send() since previous version was

100e219

required to work around to the duplication of the send() function. Added implementation to automatically initialize the identity of the logger through the use of the global `program_invocation_short_name`.

Added definition of sockaddr_un_bsd

cd22936

jart added a commit that referenced this pull request Mar 29, 2021

Remove colliding name in syscalls.sh

ae300d0

Credit goes to @fabriziobertocci for finding this. See #136.

fabriziobertocci added 2 commits April 1, 2021 09:14

Added Arity columns for the manually modified stub functions

f2b8cbd

Increased the size of the sun_path field in a sockaddr_un structure t…

5f2fce9

…o match the standard of 108 characters. Modified the bind/connect/sendto to adjust to this change

jart approved these changes Apr 2, 2021

View reviewed changes

jart merged commit 6682013 into jart:master Apr 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Added implementation for syslog related functionality. #136

Added implementation for syslog related functionality. #136

Uh oh!

fabriziobertocci commented Mar 25, 2021

Uh oh!

jart left a comment

Uh oh!

jart Mar 25, 2021

Uh oh!

fabriziobertocci Mar 25, 2021

Uh oh!

jart Mar 25, 2021

Uh oh!

fabriziobertocci Mar 25, 2021

Uh oh!

jart Mar 25, 2021

Uh oh!

fabriziobertocci Mar 25, 2021

Uh oh!

fabriziobertocci commented Mar 25, 2021

Uh oh!

fabriziobertocci commented Mar 25, 2021

Uh oh!

jart commented Mar 31, 2021

Uh oh!

fabriziobertocci commented Apr 1, 2021 via email

Uh oh!

fabriziobertocci commented Apr 1, 2021 via email •

edited

Loading

Uh oh!

jart left a comment

Uh oh!

Uh oh!

Uh oh!

Added implementation for syslog related functionality. #136

Added implementation for syslog related functionality. #136

Uh oh!

Conversation

fabriziobertocci commented Mar 25, 2021

Uh oh!

jart left a comment

Choose a reason for hiding this comment

Uh oh!

jart Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

fabriziobertocci Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

jart Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

fabriziobertocci Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

jart Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

fabriziobertocci Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

fabriziobertocci commented Mar 25, 2021

Uh oh!

fabriziobertocci commented Mar 25, 2021

Uh oh!

jart commented Mar 31, 2021

Uh oh!

fabriziobertocci commented Apr 1, 2021 via email

Uh oh!

fabriziobertocci commented Apr 1, 2021 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fabriziobertocci commented Apr 1, 2021 via email •

edited

Loading