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.

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

delivery: message_dequeue_load_from_mess has broken filename handling #135

Open
liske opened this issue Mar 7, 2025 · 0 comments
Open

Comments

@liske
Copy link

liske commented Mar 7, 2025

We observed that requeuing mails using the suggested unit from the docs does never work.

While tracing continuous coredumps in the most recent gromox 2.43 we found out that it will never work by design. We observed a continuous trashing of readdir on /var/lib/gromox/queue/mess/ with failed openat of gromox-delivery. After some gdb based analysis we found what the process does and the origin of the issue:

  1. the mess directory is scanned in mdq_thrwork and for each file which should be processed message_dequeue_load_from_mess is called in L432
  2. the filename is not passed as a parameter - instead the string is converted to a long integer using strtol
  3. the base parameter is set to 0 - so that the numeric base is derived from the string (so it is interpreted as octal notation, but it is already bricked because of the leading zero)
  4. inside of the message_dequeue_load_from_mess function the integel value is converted back (!) to a string using std::to_string => decimal representation

This loops over again and again - what a MESS!

Please consider not to convert filename strings to integer just to convert them back to strings some CPU clocks later (mind the 🌳). IMHO a good approach would be to pass always a filename (string) on message_dequeue_load_from_mess and move the to_string to the msgrcv handling, only.

liske added a commit to liske/grommunio-documentation that referenced this issue Mar 7, 2025
Using a filenames which cannot be interpreted as integer values in decimal notation
without any leading zeros does not work[1]. The **only** id which can be used save seems
to be `0`, other values can be overwritten due to[2] and the implementation
of `message_enqueue_retrieve_max_ID`[3] which is **not** free of race conditions. So
better be save than sorry.

[1] grommunio/gromox#135
[2] grommunio/gromox#136
[3] https://github.com/grommunio/gromox/blob/17ca866e731a3c19a6e66491442dd68a72b0ad39/mda/message_enqueue.cpp#L328

The code has been improved:
- always use `mv -n` to **never** replace existing files
- do not croak if `save/` is empty
liske added a commit to liske/grommunio-documentation that referenced this issue Mar 7, 2025
Using a filename which cannot be interpreted as integer value in decimal notation
without any leading zeros does not work[1]. The **only** id which can be used save seems
to be `0`, other values can be overwritten due to [2] and the implementation
of `message_enqueue_retrieve_max_ID`[3] which is **not** free of race conditions. So
better be save than sorry.

[1] grommunio/gromox#135
[2] grommunio/gromox#136
[3] https://github.com/grommunio/gromox/blob/17ca866e731a3c19a6e66491442dd68a72b0ad39/mda/message_enqueue.cpp#L328

The code has been improved:
- always use `mv -n` to **never** replace existing files
- do not croak if `save/` is empty
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

No branches or pull requests

1 participant