Prevent replacement of format strings in message by bebehei · Pull Request #365 · dunst-project/dunst

bebehei · 2017-08-28T10:32:22Z

To replace all occuring format strings inside the msg, replace_all had
been used previously. This leads to buggy behavior, if a format string
occurs in the replaced text, as the format string will get replaced
again under certain conditions.

Introducing a pointer, which skips the already replaced parts, will
prevent doubly replacing format strings from content.

Fixes #322

tsipinakis

Again, sorry for the huge delay in getting through PRs, looks like it's going to be a while until I am up and running again.

I can't do a full review currently but here are a few passing remarks looking at the code.

tsipinakis · 2017-09-04T06:05:20Z

src/notification.c

-        char* tmp;
+char *notification_replace_single_field(char *haystack, char **needle,
+                const char *replacement, enum markup_mode markup_mode) {
+


Since this is assuming that **needle is the beginning of a format flag, I think an assertion is in order to catch any bugs.

This is fixed now the line below.

Also I added another assertion, that needle points into haystack.

tsipinakis · 2017-09-04T06:06:38Z

src/notification.c

+                                                MARKUP_NO);
+                                break;
+                        default:
+                                break;


There should probably be a warning here, let's not silently ignore unknown items.

tsipinakis · 2017-09-04T06:12:01Z

test/notification.c

-        ASSERT_STR_EQ("Markup is removed and &amp; escaped", (str = notification_replace_format("%a", "<i>is removed</i> and & escaped", str, MARKUP_STRIP)));
+        substr = strchr(str, '%');
+        ASSERT_STR_EQ("Markup is removed and &amp; escaped", (str = notification_replace_single_field(str, &substr, "<i>is removed</i> and & escaped", MARKUP_STRIP)));
+        ASSERT_EQ(strlen("Markup is removed and &amp; escaped"), substr - str);


Why use strlen here but hardcode all the others?

tsipinakis · 2017-09-04T06:14:06Z

src/notification.c

-char *notification_replace_format(const char *needle, const char *replacement,
-                                  char *haystack, enum markup_mode markup_mode) {
-        char* tmp;
+char *notification_replace_single_field(char *haystack, char **needle,


If we going to use a double pointer for the needle, I think it'd be better to also use it for the haystack and avoid the awkward assignments in notification_init.

Is this more "C-ish" than the current signature? I changed it, now for evaluation.

bebehei · 2017-09-04T11:34:41Z

~~Found a bug: format string <some stuff>%A creates endless loop~~

Fixed.

tsipinakis · 2017-09-06T18:08:25Z

Found a bug

I'd love to have a regression test for this but that means it'd also print the warning with it. I've opened #368 to look into somehow controlling how logging works.

tsipinakis · 2017-09-06T18:38:31Z

src/notification.c

+        assert(*needle >= *haystack);
+        assert(*needle - *haystack <= strlen(*haystack));
+
+        char* input;


While currently it's already in a similar way in master I think it's worth fixing these nitpicks since we're refactoring this anyway:

There is no reason to have this declaration here and not a few lines bellow where input is first used

While it's pretty inconsistent across the project, we usually put the asterisk next to the variable name and not the type e.g. char *input.
The rationale for this is simply that it gets confusing when using commas, for example in char* a,b a is a pointer while b is not.

While it's pretty inconsistent across the project, we usually put the asterisk next to the variable name and not the type e.g. char input.
The rationale for this is simply that it gets confusing when using commas, for example in char a,b a is a pointer while b is not.

Thanks for answering this question. I knew this was the C-specific equivalent of "tabs vs spaces", so I kept avoiding it, because I thought I could not understand all arguments. But thanks it's very clear now.

There is no reason to have this declaration here and not a few lines bellow where input is first used

So, while we're at these "religious" questions:

char *string = function_returning_string();

[begin of the block] char *string; [maybe some code] string = function_returning_string();

I've seen both styles here in the project, but I haven't found a scheme. My observations had been, that a C-programmer rather uses the 2nd style.

What's better or when use what?

tsipinakis · 2017-09-06T18:50:30Z

src/notification.c

+        assert(*needle[0] == '%');
+        // needle has to point into haystack
+        assert(*needle >= *haystack);
+        assert(*needle - *haystack <= strlen(*haystack));


This assertion should probably test for less than not less-than-or-equal: We don't want a trailing % at the end of the string, if anything it might trip over string_replace_at into undefined behaviour.

tsipinakis · 2017-09-06T18:54:59Z

src/notification.c

+                                break;
+                        default:
+                                fprintf(stderr, "WARNING: format_string %%%c"
+                                                " is unknown\n", substr[1]);


There should be a different message if substr[1] is \0, aka we're at the end of the string.

e.g. WARNING: Trailing % mark in format

bebehei · 2017-09-06T20:20:46Z

I'd love to have a regression test

I'd love that, too. But I consider this part of the function as "not testable yet". The function is too big and needs to be refactored to have a senseful unittest.

tsipinakis · 2017-09-07T10:54:45Z

Can you squash 1aaf109 into ac5542a and resolve the conflicts?

To replace all occuring format strings inside the msg, replace_all had been used previously. This leads to buggy behavior, if a format string occurs in the replaced text, as the format string will get replaced again under certain conditions. Introducing a pointer, which skips the already replaced parts, will prevent doubly replacing format strings from content. Fixes dunst-project#322

bebehei · 2017-09-07T13:44:13Z

Rebased and LGTM.

tsipinakis · 2017-09-09T06:08:47Z

LGTM 👍

Redo dunst-project#354, as it got pushed out in dunst-project#365

bebehei force-pushed the issue-322 branch 2 times, most recently from 0b0e257 to 5f41365 Compare August 28, 2017 11:35

bebehei changed the title ~~Prevent replacement format strings in message content~~ Prevent replacement of format strings in message Aug 28, 2017

tsipinakis reviewed Sep 4, 2017

View reviewed changes

bebehei force-pushed the issue-322 branch from e2369b1 to 0d8c585 Compare September 4, 2017 11:21

bebehei force-pushed the issue-322 branch 7 times, most recently from 256f9ee to 31efd01 Compare September 6, 2017 11:45

tsipinakis mentioned this pull request Sep 6, 2017

Additional logging & handling logging during tests #368

Closed

3 tasks

tsipinakis requested changes Sep 6, 2017

View reviewed changes

bebehei force-pushed the issue-322 branch from 31efd01 to 1110b04 Compare September 6, 2017 20:21

bebehei mentioned this pull request Sep 6, 2017

Refactor star scheme of pointers #369

Merged

tsipinakis approved these changes Sep 7, 2017

View reviewed changes

bebehei added 3 commits September 7, 2017 15:25

Interpret '%%' as literal '%' in format

4b7f656

Use double pointers for haystack

e16117c

bebehei force-pushed the issue-322 branch from 1110b04 to e16117c Compare September 7, 2017 13:27

tsipinakis merged commit 29aa4c7 into dunst-project:master Sep 9, 2017

bebehei deleted the issue-322 branch September 9, 2017 15:52

bebehei added a commit to bebehei/dunst that referenced this pull request Oct 6, 2017

Work around non-gnu basename implementations

d33a4fe

Redo dunst-project#354, as it got pushed out in dunst-project#365

bebehei mentioned this pull request Oct 6, 2017

Work around non-gnu basename implementations #403

Merged

bebehei added a commit to bebehei/dunst that referenced this pull request Oct 6, 2017

Work around non-gnu basename implementations

9aba546

Redo dunst-project#354, as it got pushed out in dunst-project#365

Conversation

bebehei commented Aug 28, 2017

Uh oh!

tsipinakis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bebehei commented Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tsipinakis commented Sep 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsipinakis Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bebehei commented Sep 6, 2017

Uh oh!

tsipinakis commented Sep 7, 2017

Uh oh!

bebehei commented Sep 7, 2017

Uh oh!

tsipinakis commented Sep 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tsipinakis left a comment •

edited

Loading

bebehei commented Sep 4, 2017 •

edited

Loading

tsipinakis Sep 6, 2017 •

edited

Loading