limit length of single fields in notification #332

bebehei · 2017-07-07T11:37:41Z

This would be my 2 cents to fix #248 and remove the last blocker to have a new release.

This PR adds the option max_fieldlength to have a maximum of specific chars per field. So overlong messages will get truncated to the specified length.

Also if it's not wanted to have a limit, a minus-value can be chosen to skip the cutting.

I'm not sure yet, what a reasonable default would be. I've used 100, but I'd like to have other's opinions here. Alternatively -1 (=> no limit) would also be a good default.

tsipinakis · 2017-07-07T12:12:07Z

I'm not sure a user visible setting is the best approach here. I doubt that truncating based on the number of characters in a notification has any real use case other than preventing the DoS that happens in #248.

I think a macro set to a high enough value that it's unlikely for anyone to bump into(Say 5000?) is the best and we can adjust that as needed.

bebehei · 2017-07-08T00:15:17Z

I thought long about this. But yes, it's better to have this inside a macro and not expose it to the user.

IMHO a good notification is always less than 100 chars. If it's a long text, nevertheless you click on the notification to open the messenger and read it there. The introduced option would have made the truncation possible, which also prevents DoSing dunst.

But it just creates too much noise in dunstrc.

tsipinakis · 2017-07-08T08:23:10Z

src/utils.c

+        if (maxlen < 0 || strlen(string) < maxlen) {
+                buffer = string;
+        } else {
+                size_t buffer_maxlen = maxlen;


Why create a new variable instead of making the type of maxlen be size_t in the function definition? And with this change, there should be no need to check if maxlen < 0 since size_t is unsigned.

tsipinakis · 2017-07-08T08:31:25Z

src/utils.c

+                buffer = string;
+        } else {
+                size_t buffer_maxlen = maxlen;
+                buffer = calloc(buffer_maxlen, sizeof(char));


We're using glib so instead of calloc g_malloc0 should be used but since we already know we are going to fill the entire buffer there is no reason to zero it out so g_malloc would probably be the correct function to use here.

Thanks for pointing this out. My C-Skills are quite limited and I have not known the differences of all available allocation-functions yet.

tsipinakis · 2017-07-08T08:33:55Z

src/utils.c

+        } else {
+                size_t buffer_maxlen = maxlen;
+                buffer = calloc(buffer_maxlen, sizeof(char));
+                strncpy(buffer, string, buffer_maxlen);


Quoting from strcpy(3)

If there is no terminating null byte in the first n bytes of src, strncpy() produces an unterminated string in dest.

In other words at this point buffer is unterminated. The last character should be replaced with a null byte.

Good catch. Thanks.

tsipinakis · 2017-07-08T08:36:12Z

src/utils.h

@@ -9,6 +9,9 @@ char *string_replace_char(char needle, char replacement, char *haystack);
 char *string_replace_all(const char *needle, const char *replacement,
                         char *haystack);

+/* Cut the given string at buffer_maxlen */


The parameter is named maxlen not buffer_maxlen

bebehei · 2017-07-08T11:33:41Z

Thanks for the feedback, @tsipinakis. I processed all requested changes.

bebehei · 2017-07-08T11:40:57Z

src/notification.h

@@ -12,6 +12,8 @@
 #define NORM 1
 #define CRIT 2

+#define MAX_CHARS 5000


@tsipinakis Is there any naming convention for macros? IMHO MAX_CHARS is too generic.

Not really, but I do agree that it's too generic maybe NOTIFICATION_MAX_LENGTH?

tsipinakis · 2017-07-08T13:10:09Z

src/notification.c

@@ -305,11 +305,11 @@ int notification_init(notification * n, int id)
        n->urls = notification_extract_markup_urls(&(n->body));

        n->msg = string_replace_all("\\n", "\n", g_strdup(n->format));
-        n->msg = notification_replace_format("%a", n->appname, n->msg,
+        n->msg = notification_replace_format("%a", string_cut_maxlen(n->appname, MAX_CHARS), n->msg,


Oops, I missed this earlier but this creates a memory leak since the memory you allocate in string_cut_maxlen is never freed. Additionally I'm not sure if having a limit per-field is the correct option. In my opinion I'd change this to a single string_cut_maxlen call at the end of the format replacements.

Also, it should probably be placed after the icon processing, since we have flags to add the icon path to the format and a huge icon path would cause the same problem.

tsipinakis · 2017-07-08T13:11:19Z

src/utils.h

@@ -2,13 +2,18 @@
 #ifndef DUNST_UTILS_H
 #define DUNST_UTILS_H

+#include <stdlib.h>


size_t is defined in stddef.h. While stddlib includes stddef it's usually best practice to use the more specific header file.

tsipinakis · 2017-07-08T13:12:05Z

src/utils.h

 /* replace all occurences of the character needle with the character replacement in haystack */
 char *string_replace_char(char needle, char replacement, char *haystack);

 /* replace all occurrences of needle with replacement in haystack */
 char *string_replace_all(const char *needle, const char *replacement,
                         char *haystack);

+/* Cut the given string at maxlen */
+char* string_cut_maxlen(char* string, size_t maxlen);


Small nitpick, but I think string_truncate would be a better name for this function.

Displaying too heavy notifications can DoS dunst. For example bad programs, which pipe raw image data into the notification. Limiting the maximum character length to 5000 circumvents this. 5000 should be ridiculously high to prevent DoS while still not truncating all correct notifications.

bebehei · 2017-07-09T14:39:28Z

@tsipinakis Thank you for your feedback.

I dropped the stuff in utils.c/h, it limits the whole message size now.

Do we care about broken HTML if the output got truncated?

tsipinakis · 2017-07-09T16:44:26Z

I dropped the stuff in utils.c/h, it limits the whole message size now.

That's much better indeed, good call.

Do we care about broken HTML if the output got truncated?

I don't think we should, any notification that reaches that limit will probably clip the monitor edges anyway. This is just a stopgap to stop dunst from freezing in case something messes up and sends much more data than it should.

Thanks!

bebehei · 2017-07-09T16:49:14Z

Well, then will be there a 1.2 and close #263 ?

tsipinakis · 2017-07-09T16:55:42Z

Well, then will be there a 1.2 and close #263 ?

I posted an update in #263 with the status of the release.

bebehei mentioned this pull request Jul 7, 2017

No character limit with Mumble #248

Closed

tsipinakis requested changes Jul 8, 2017

View reviewed changes

bebehei force-pushed the fieldlength-max branch from 53abfa9 to b293771 Compare July 8, 2017 11:31

bebehei commented Jul 8, 2017

View reviewed changes

tsipinakis requested changes Jul 8, 2017

View reviewed changes

bebehei force-pushed the fieldlength-max branch 2 times, most recently from 4805aad to 52d2885 Compare July 9, 2017 14:21

bebehei force-pushed the fieldlength-max branch from 52d2885 to 717c747 Compare July 9, 2017 14:35

tsipinakis approved these changes Jul 9, 2017

View reviewed changes

tsipinakis merged commit d206f24 into dunst-project:master Jul 9, 2017

bebehei deleted the fieldlength-max branch August 1, 2017 11:11

bebehei mentioned this pull request Oct 27, 2023

Dunst maintenance status #1117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

limit length of single fields in notification #332

limit length of single fields in notification #332

bebehei commented Jul 7, 2017

tsipinakis commented Jul 7, 2017

bebehei commented Jul 8, 2017

tsipinakis Jul 8, 2017

bebehei Jul 8, 2017

tsipinakis Jul 8, 2017

bebehei Jul 8, 2017

tsipinakis Jul 8, 2017

bebehei Jul 8, 2017

tsipinakis Jul 8, 2017

bebehei Jul 8, 2017

bebehei commented Jul 8, 2017

bebehei Jul 8, 2017

tsipinakis Jul 8, 2017

tsipinakis Jul 8, 2017

tsipinakis Jul 8, 2017

tsipinakis Jul 8, 2017

bebehei commented Jul 9, 2017

tsipinakis commented Jul 9, 2017 •

edited

Loading

bebehei commented Jul 9, 2017

tsipinakis commented Jul 9, 2017

limit length of single fields in notification #332

limit length of single fields in notification #332

Conversation

bebehei commented Jul 7, 2017

tsipinakis commented Jul 7, 2017

bebehei commented Jul 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bebehei commented Jul 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bebehei commented Jul 9, 2017

tsipinakis commented Jul 9, 2017 • edited Loading

bebehei commented Jul 9, 2017

tsipinakis commented Jul 9, 2017

tsipinakis commented Jul 9, 2017 •

edited

Loading