[Dnsmasq-discuss] Extension to hosts-dir and dhcohosts-dir

[Andy Hawkins]

Hi all,

Would it be possible to extend hosts-dir and dhcphosts-dir to also allow the
specification of a file mask to process within those directories?

I'm finding that when I edit files in those directories, my editor creates a
backup file (original file name with ~ appended) and these backup files are
then processed by dnsmasq leading to a duplicate.

Would be nice if they could be made to work the same was as conf-dir does.

Thanks

Andy

[Kurt H Maier]

Post by Andy Hawkins
I'm finding that when I edit files in those directories, my editor creates a
backup file (original file name with ~ appended) and these backup files are
then processed by dnsmasq leading to a duplicate.

You should fix the editor; that behavior is dangerous for other reasons,
similar to the ones outlined here:
http://openwall.com/lists/oss-security/2017/11/27/2

In essence, if you're going to be editing service configurations on a
live system, you should make sure your editor does not do this.

In vim or emacs it's just a 'set backupdir' or 'setq
backup-directory-alist' away.

khm

[Andy Hawkins]

Hi,

Post by Kurt H Maier
You should fix the editor; that behavior is dangerous for other reasons,
http://openwall.com/lists/oss-security/2017/11/27/2

I take your point. However, given that the facility is available for config
files, I don't see any reason why it shouldn't be extended to other
directories that contain files that are designed to be modified while
dnsmasq is running.

Andy

[Geert Stappers]

Post by Andy Hawkins

Post by Kurt H Maier
You should fix the editor; that behavior is dangerous for other reasons,
http://openwall.com/lists/oss-security/2017/11/27/2

I take your point. However, given that the facility is available for config
files, I don't see any reason why it shouldn't be extended to other
directories that contain files that are designed to be modified while
dnsmasq is running.

I do read that statement as 80% of needed source code already present.
Craft the missing source code into a patch, posted to here
and see what happens.


Groeten
Geert Stappers

[Andy Hawkins]

Hi,

Post by Geert Stappers
I do read that statement as 80% of needed source code already present.
Craft the missing source code into a patch, posted to here
and see what happens.

I decided to do exactly that, and started looking at the code. It looks like
it should already be ignoring files that end in a ~. However, on my version
it's not.

In inotify.c, around line 236 is the following code block:

/* ignore emacs backups and dotfiles */
if (in->len == 0 ||
in->name[in->len - 1] == '~' ||
(in->name[0] == '#' && in->name[in->len - 1] == '#') ||
in->name[0] == '.')
continue;

However, if I create a file called 'fred~' in the directory I've specified
using dhcp-hostsdir I still get an event in syslog that shows this file is
being processed:

Feb 11 11:14:34 xcp-gateway dnsmasq[1039]: inotify, new or changed file
/etc/dnsmasq/dhcp-hosts.d/fred~

This suggest that either I'm reading the code incorrectly (perfectly
possible!) or that the code isn't doing what it is supposed to do?

Which is it? Anyone?

Thanks

Andy

[Andy Hawkins]

Hi,

Post by Andy Hawkins
/* ignore emacs backups and dotfiles */
if (in->len == 0 ||
in->name[in->len - 1] == '~' ||
(in->name[0] == '#' && in->name[in->len - 1] == '#') ||
in->name[0] == '.')
continue;
However, if I create a file called 'fred~' in the directory I've specified
using dhcp-hostsdir I still get an event in syslog that shows this file is
Feb 11 11:14:34 xcp-gateway dnsmasq[1039]: inotify, new or changed file
/etc/dnsmasq/dhcp-hosts.d/fred~

Ok, I've done some debugging. I added the following lines:

my_syslog(LOG_INFO, "ADH: len: %d", in->len);
my_syslog(LOG_INFO, "ADH: name: %s", in->name);
my_syslog(LOG_INFO, "ADH: last char: %c", in->name[in->len - 1]);

And I get the following output:

dnsmasq: ADH: len: 16
dnsmasq: ADH: name: fred4~
dnsmasq: ADH: last char:

So, it appears that the length in in->len is being interpreted correctly.

According to the inotify man page:

The len field counts all of the bytes in name, including the null
bytes; the length of each inotify_event structure is thus
sizeof(struct inotify_event)+len.

So in fact, 'len' seems to be a fixed length, irrespective of the length of
the file name in the 'name' field.

It looks like the check should actually be something like:

/* ignore emacs backups and dotfiles */
if (in->len == 0 ||
in->name[strlen(in->name) - 1] == '~' ||
(in->name[0] == '#' && in->name[strlen(in->name) - 1] == '#') ||
in->name[0] == '.')

I guess you may need to check that there's a null in the name somewhere
before using strlen, otherwise you might end up running off the end of the
string. I don't know inotify well enough to know if there's guaranteed to be
a null in there somewhere. The manpage does say that the name field is null
terminated, but I don't know if that's guaranteed or not.

I could have a look at submitting a patch, but my editor is showing some
very strange indentation of the source, so I suspect I have my tab settings
incorrect. What is the standard setting for tabs on the dnasmasq source
files?

Thanks

Andy

[Dominik Derigs]

Hey Andy (and list),

According to the inotify man page, the name buffer will always be
null-terminated. Furthermore, the name buffer seems to be allocated in
chunks of 16 bytes. I have not found an official confirmation for that.

I the way to go would be: char lastchar = in->name[strlen(in->name)-1];

I have extended your test code by the following two lines:

if(in->name == NULL) continue;
printf("DD: strlen: %lu\n", strlen(in->name));
char lastchar = in->name[strlen(in->name)-1];
printf("DD: strlen last char: %c\n", lastchar);

Here are results obtained with your + my debug output:

For a short filename without ~

ADH: len: 16
ADH: name: shortfilename
ADH: last char:
DD: strlen: 13
DD: strlen last char: e

For a short filename with ~

ADH: len: 16
ADH: name: shortfilename~
ADH: last char:
DD: strlen: 14
DD: strlen last char: ~

For a long filename without ~

ADH: len: 32
ADH: name: veryverylongfilename
ADH: last char:
DD: strlen: 20
DD: strlen last char: e

For a long filename with ~

ADH: len: 32
ADH: name: veryverylongfilename~
ADH: last char:
DD: strlen: 21
DD: strlen last char: ~

Best regards,
Dominik

Post by Andy Hawkins
Hi,

Post by Andy Hawkins
/* ignore emacs backups and dotfiles */
if (in->len == 0 ||
in->name[in->len - 1] == '~' ||
(in->name[0] == '#' && in->name[in->len - 1] == '#') ||
in->name[0] == '.')
continue;
However, if I create a file called 'fred~' in the directory I've specified
using dhcp-hostsdir I still get an event in syslog that shows this file is
Feb 11 11:14:34 xcp-gateway dnsmasq[1039]: inotify, new or changed file
/etc/dnsmasq/dhcp-hosts.d/fred~

my_syslog(LOG_INFO, "ADH: len: %d", in->len);
my_syslog(LOG_INFO, "ADH: name: %s", in->name);
my_syslog(LOG_INFO, "ADH: last char: %c", in->name[in->len - 1]);
dnsmasq: ADH: len: 16
dnsmasq: ADH: name: fred4~
So, it appears that the length in in->len is being interpreted correctly.
The len field counts all of the bytes in name, including the null
bytes; the length of each inotify_event structure is thus
sizeof(struct inotify_event)+len.
So in fact, 'len' seems to be a fixed length, irrespective of the length of
the file name in the 'name' field.
/* ignore emacs backups and dotfiles */
if (in->len == 0 ||
in->name[strlen(in->name) - 1] == '~' ||
(in->name[0] == '#' && in->name[strlen(in->name) - 1] == '#') ||
in->name[0] == '.')
I guess you may need to check that there's a null in the name somewhere
before using strlen, otherwise you might end up running off the end of the
string. I don't know inotify well enough to know if there's guaranteed to be
a null in there somewhere. The manpage does say that the name field is null
terminated, but I don't know if that's guaranteed or not.
I could have a look at submitting a patch, but my editor is showing some
very strange indentation of the source, so I suspect I have my tab settings
incorrect. What is the standard setting for tabs on the dnasmasq source
files?
Thanks
Andy
_______________________________________________
Dnsmasq-discuss mailing list
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

[Dominik Derigs]

Forgot to attach the proposed patch.

Best,
Dominik

Post by Dominik Derigs
Hey Andy (and list),
According to the inotify man page, the name buffer will always be
null-terminated. Furthermore, the name buffer seems to be allocated in
chunks of 16 bytes. I have not found an official confirmation for that.
I the way to go would be: char lastchar = in->name[strlen(in->name)-1];
if(in->name == NULL) continue;
printf("DD: strlen: %lu\n", strlen(in->name));
char lastchar = in->name[strlen(in->name)-1];
printf("DD: strlen last char: %c\n", lastchar);
For a short filename without ~
ADH: len: 16
ADH: name: shortfilename
DD: strlen: 13
DD: strlen last char: e
For a short filename with ~
ADH: len: 16
ADH: name: shortfilename~
DD: strlen: 14
DD: strlen last char: ~
For a long filename without ~
ADH: len: 32
ADH: name: veryverylongfilename
DD: strlen: 20
DD: strlen last char: e
For a long filename with ~
ADH: len: 32
ADH: name: veryverylongfilename~
DD: strlen: 21
DD: strlen last char: ~
Best regards,
Dominik

Post by Andy Hawkins
Hi,

Post by Andy Hawkins
/* ignore emacs backups and dotfiles */
if (in->len == 0 ||
in->name[in->len - 1] == '~' ||
(in->name[0] == '#' && in->name[in->len - 1] == '#') ||
in->name[0] == '.')
continue;
However, if I create a file called 'fred~' in the directory I've specified
using dhcp-hostsdir I still get an event in syslog that shows this file is
Feb 11 11:14:34 xcp-gateway dnsmasq[1039]: inotify, new or changed file
/etc/dnsmasq/dhcp-hosts.d/fred~

my_syslog(LOG_INFO, "ADH: len: %d", in->len);
my_syslog(LOG_INFO, "ADH: name: %s", in->name);
my_syslog(LOG_INFO, "ADH: last char: %c", in->name[in->len - 1]);
dnsmasq: ADH: len: 16
dnsmasq: ADH: name: fred4~
So, it appears that the length in in->len is being interpreted correctly.
The len field counts all of the bytes in name, including the null
bytes; the length of each inotify_event structure is thus
sizeof(struct inotify_event)+len.
So in fact, 'len' seems to be a fixed length, irrespective of the length of
the file name in the 'name' field.
/* ignore emacs backups and dotfiles */
if (in->len == 0 ||
in->name[strlen(in->name) - 1] == '~' ||
(in->name[0] == '#' && in->name[strlen(in->name) - 1] == '#') ||
in->name[0] == '.')
I guess you may need to check that there's a null in the name somewhere
before using strlen, otherwise you might end up running off the end of the
string. I don't know inotify well enough to know if there's guaranteed to be
a null in there somewhere. The manpage does say that the name field is null
terminated, but I don't know if that's guaranteed or not.
I could have a look at submitting a patch, but my editor is showing some
very strange indentation of the source, so I suspect I have my tab settings
incorrect. What is the standard setting for tabs on the dnasmasq source
files?
Thanks
Andy
_______________________________________________
Dnsmasq-discuss mailing list
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

[Lonnie Abelbeck]

Dominik,

I'm thinking you do not want to call
--
in->name[strlen(in->name)-1];
--
before testing for "if (in->len == 0"

Lonnie

Post by Dominik Derigs
Forgot to attach the proposed patch.
Best,
Dominik

Post by Dominik Derigs
Hey Andy (and list),
According to the inotify man page, the name buffer will always be
null-terminated. Furthermore, the name buffer seems to be allocated in
chunks of 16 bytes. I have not found an official confirmation for that.
I the way to go would be: char lastchar = in->name[strlen(in->name)-1];
if(in->name == NULL) continue;
printf("DD: strlen: %lu\n", strlen(in->name));
char lastchar = in->name[strlen(in->name)-1];
printf("DD: strlen last char: %c\n", lastchar);
For a short filename without ~
ADH: len: 16
ADH: name: shortfilename
DD: strlen: 13
DD: strlen last char: e
For a short filename with ~
ADH: len: 16
ADH: name: shortfilename~
DD: strlen: 14
DD: strlen last char: ~
For a long filename without ~
ADH: len: 32
ADH: name: veryverylongfilename
DD: strlen: 20
DD: strlen last char: e
For a long filename with ~
ADH: len: 32
ADH: name: veryverylongfilename~
DD: strlen: 21
DD: strlen last char: ~
Best regards,
Dominik

Post by Andy Hawkins
Hi,

Post by Andy Hawkins
/* ignore emacs backups and dotfiles */
if (in->len == 0 ||
in->name[in->len - 1] == '~' ||
(in->name[0] == '#' && in->name[in->len - 1] == '#') ||
in->name[0] == '.')
continue;
However, if I create a file called 'fred~' in the directory I've specified
using dhcp-hostsdir I still get an event in syslog that shows this file is
Feb 11 11:14:34 xcp-gateway dnsmasq[1039]: inotify, new or changed file
/etc/dnsmasq/dhcp-hosts.d/fred~

my_syslog(LOG_INFO, "ADH: len: %d", in->len);
my_syslog(LOG_INFO, "ADH: name: %s", in->name);
my_syslog(LOG_INFO, "ADH: last char: %c", in->name[in->len - 1]);
dnsmasq: ADH: len: 16
dnsmasq: ADH: name: fred4~
So, it appears that the length in in->len is being interpreted correctly.
The len field counts all of the bytes in name, including the null
bytes; the length of each inotify_event structure is thus
sizeof(struct inotify_event)+len.
So in fact, 'len' seems to be a fixed length, irrespective of the length of
the file name in the 'name' field.
/* ignore emacs backups and dotfiles */
if (in->len == 0 ||
in->name[strlen(in->name) - 1] == '~' ||
(in->name[0] == '#' && in->name[strlen(in->name) - 1] == '#') ||
in->name[0] == '.')
I guess you may need to check that there's a null in the name somewhere
before using strlen, otherwise you might end up running off the end of the
string. I don't know inotify well enough to know if there's guaranteed to be
a null in there somewhere. The manpage does say that the name field is null
terminated, but I don't know if that's guaranteed or not.
I could have a look at submitting a patch, but my editor is showing some
very strange indentation of the source, so I suspect I have my tab settings
incorrect. What is the standard setting for tabs on the dnasmasq source
files?
Thanks
Andy
_______________________________________________
Dnsmasq-discuss mailing list
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

<inotfy.patch>_______________________________________________
Dnsmasq-discuss mailing list
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

[Andy Hawkins]

Hi,

Post by Andy Hawkins
I could have a look at submitting a patch, but my editor is showing some
very strange indentation of the source, so I suspect I have my tab settings
incorrect. What is the standard setting for tabs on the dnasmasq source
files?

Here's an attempt at a patch. If it needs to be in a different format, then
please let me know. The changes are minimal however, so applying the patch
manually should be trivial.

[***@xcp-dev dnsmasq (hosts-dirs *)]$ git diff --ignore-space-at-eol
diff --git a/src/inotify.c b/src/inotify.c
old mode 100644
new mode 100755
index eda1d56..a655fe2
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -235,8 +235,8 @@ int inotify_check(time_t now)

/* ignore emacs backups and dotfiles */
if (in->len == 0 ||
- in->name[in->len - 1] == '~' ||
- (in->name[0] == '#' && in->name[in->len - 1] == '#') ||
+ in->name[strlen(in->name) - 1] == '~' ||
+ (in->name[0] == '#' && in->name[strlen(in->name) - 1] == '#') ||
in->name[0] == '.')
continue;

Hope that helps.

Andy

[john doe]

Post by Andy Hawkins
Hi,

Post by Andy Hawkins
I could have a look at submitting a patch, but my editor is showing some
very strange indentation of the source, so I suspect I have my tab settings
incorrect. What is the standard setting for tabs on the dnasmasq source
files?

Here's an attempt at a patch. If it needs to be in a different format, then
please let me know. The changes are minimal however, so applying the patch
manually should be trivial.
diff --git a/src/inotify.c b/src/inotify.c
old mode 100644
new mode 100755

Is the change of the mode intentionel (from 644 to 755)?

Post by Andy Hawkins
index eda1d56..a655fe2
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -235,8 +235,8 @@ int inotify_check(time_t now)
/* ignore emacs backups and dotfiles */
if (in->len == 0 ||
- in->name[in->len - 1] == '~' ||
- (in->name[0] == '#' && in->name[in->len - 1] == '#') ||
+ in->name[strlen(in->name) - 1] == '~' ||
+ (in->name[0] == '#' && in->name[strlen(in->name) - 1] == '#') ||
in->name[0] == '.')
continue;
Hope that helps.
Andy
_______________________________________________
Dnsmasq-discuss mailing list
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

[Andy Hawkins]

Hi,

Post by john doe

Post by Andy Hawkins
diff --git a/src/inotify.c b/src/inotify.c
old mode 100644
new mode 100755

Is the change of the mode intentionel (from 644 to 755)?

No. Probably a combination of my editor and it being accessed via Samba.

The key change is to the content of inotify.c.

Andy

[Geert Stappers]

Post by Andy Hawkins
Hi,

Post by john doe

Post by Andy Hawkins
diff --git a/src/inotify.c b/src/inotify.c
old mode 100644
new mode 100755

Is the change of the mode intentionel (from 644 to 755)?

No. Probably a combination of my editor and it being accessed via Samba.
The key change is to the content of inotify.c.
Andy

FWIW I'm formatting the patch so it be `git am`
It will have Andy's name, but no sign-off.


Groeten
Geert Stappers

[Andy Hawkins]

Ignoring backup files from editor didn't work for me.

After modification of src/inotify.c where twice an
in->len
was replaced by
strlen(in->name)
it did work as expected.
---
src/inotify.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/inotify.c b/src/inotify.c
index eda1d56..45c730a 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -235,8 +235,8 @@ int inotify_check(time_t now)

/* ignore emacs backups and dotfiles */
if (in->len == 0 ||
- in->name[in->len - 1] == '~' ||
- (in->name[0] == '#' && in->name[in->len - 1] == '#') ||
+ in->name[strlen(in->name) - 1] == '~' ||
+ (in->name[0] == '#' && in->name[strlen(in->name) - 1] == '#') ||
in->name[0] == '.')
continue;

[Andy Hawkins]

Hi,

For clarity, I didn't write the below, Geert Stappers did, trying to be
helpful.

I'll try to generate a correctly formatted patch, and send it through
myself.

Thanks

Andy

Post by Andy Hawkins
Ignoring backup files from editor didn't work for me.
After modification of src/inotify.c where twice an
in->len
was replaced by
strlen(in->name)
it did work as expected.
---
src/inotify.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/inotify.c b/src/inotify.c
index eda1d56..45c730a 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -235,8 +235,8 @@ int inotify_check(time_t now)
/* ignore emacs backups and dotfiles */
if (in->len == 0 ||
- in->name[in->len - 1] == '~' ||
- (in->name[0] == '#' && in->name[in->len - 1] == '#') ||
+ in->name[strlen(in->name) - 1] == '~' ||
+ (in->name[0] == '#' && in->name[strlen(in->name) - 1] == '#') ||
in->name[0] == '.')
continue;

[Andy Hawkins]

Use strlen to determine the length of the filename returned by
inotify, as in->len refers to the length of the buffer containing
the name, not the length of the name itself.

http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2018q1/011950.html

Signed-off-by: Andy Hawkins <***@gently.org.uk>
---
src/inotify.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/inotify.c b/src/inotify.c
index eda1d56..45c730a 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -235,8 +235,8 @@ int inotify_check(time_t now)

/* ignore emacs backups and dotfiles */
if (in->len == 0 ||
- in->name[in->len - 1] == '~' ||
- (in->name[0] == '#' && in->name[in->len - 1] == '#') ||
+ in->name[strlen(in->name) - 1] == '~' ||
+ (in->name[0] == '#' && in->name[strlen(in->name) - 1] == '#') ||
in->name[0] == '.')
continue;

[Simon Kelley]

Patch applied, with two modifications.

1) I added a check for strlen(in->name) not being zero. I don't know if
inotify could give us a zero-length filename, but if it did, we'd make
an out-of-bounds array reference to in->name[-1]. I also saved the
return from strlen(in->name) in a variable, rather than call strlen
three times.

2) Move the resolv-file test to after the dotfiles test. Only relevant
if someone configures a resolv file name which looks like an editor
backup file, but at least in's consistent. Also allows removal of a
now-redundant in->len == 0 test.


Cheers,

Simon.

Post by Andy Hawkins
Use strlen to determine the length of the filename returned by
inotify, as in->len refers to the length of the buffer containing
the name, not the length of the name itself.
http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2018q1/011950.html

[Andy Hawkins]

Hi,

Post by Simon Kelley
Patch applied, with two modifications.
1) I added a check for strlen(in->name) not being zero. I don't know if
inotify could give us a zero-length filename, but if it did, we'd make
an out-of-bounds array reference to in->name[-1]. I also saved the
return from strlen(in->name) in a variable, rather than call strlen
three times.
2) Move the resolv-file test to after the dotfiles test. Only relevant
if someone configures a resolv file name which looks like an editor
backup file, but at least in's consistent. Also allows removal of a
now-redundant in->len == 0 test.

Is it possible for in->len to be zero? The man page for inotify does say the
name is optional, so I assumed that this was what that test was checking
for?

I'll take a look at your version of the code in git and see how it looks
compared to mine.

Thanks

Andy

[Andy Hawkins]

Hi,

Post by Andy Hawkins
I'll take a look at your version of the code in git and see how it looks
compared to mine.

Ok, things are a bit clearer after looking at the actual commit.

My only comment (and it's a fairly personal one) is that I generally dislike
seeing variable assignments inside if statements. However, I can see the
reason for doing it in this case, as if you carried out the strlen outside
the 'if', then you'd need to put that in an 'if' of its own, checking that
in->len wasn't zero.

Thanks for including the fix however, appreciated. What are the chances of
this ending up in a package in the current debian stable?

Thanks again

Andy

[Andy Hawkins]

Hi,

Post by Geert Stappers
FWIW I'm formatting the patch so it be `git am`

Is this the 'correct' way to do it? I couldn't really find any information
on how to contribute to dnsmasq.

Post by Geert Stappers
It will have Andy's name, but no sign-off.

Is that something I need to do?

Thanks

Andy

[john doe]

Post by Andy Hawkins
Hi,

Post by Geert Stappers
FWIW I'm formatting the patch so it be `git am`

Is this the 'correct' way to do it? I couldn't really find any information
on how to contribute to dnsmasq.

Post by Geert Stappers
It will have Andy's name, but no sign-off.

Is that something I need to do?

Idealy, you would clone the dnsmasq repository, then you would do your
changes then commit (git commit -s) them and finaly send the patch.

[Andy Hawkins]

Hi,

Post by john doe
Idealy, you would clone the dnsmasq repository, then you would do your
changes then commit (git commit -s) them and finaly send the patch.

I've already done most of that. I posted the output of git diff in an
earlier message.

What git command should I use to generate the appropriate patch?

Thanks

Andy

[Geert Stappers]

Post by Geert Stappers
FWIW I'm formatting the patch so it be `git am`

} FWIW I'm formatting the patch so it can be `git am` processed
Is this the 'correct' way to do it?
I couldn't really find any information on how to contribute to dnsmasq.

(-:

Sending patches that "git apply-mail" can handle, brings often success.
I think it is because project owner doesn't need to spend
brain power on creating a commit message, pasting in authors name.

Post by Geert Stappers
It will have Andy's name, but no sign-off.

Is that something I need to do?

If you agree with it. I, who reformatted the patch and wrote
a commit message, am in no postition to sign-off with Andy's name.
Sending an email in his name already feels wrong ...


My previous message

Post by Geert Stappers
FWIW I'm formatting the patch so it can be `git am` processed
It will have Andy's name, but no sign-off.

in other words:
|Yes, dnsmasq in nice software. I do use it, I do want to contribute.
|I do follow the mailinglist. I have seen a patch on the mailinglist.
|Also seen that it will take Simon Kelley more then needed effort
|to be applied to the git repository. So I announced the reformatting
|and use (abuse?) of Andy's name in the upcoming e-mail.


Groeten
Geert Stappers

[Will Parsons]

Post by Geert Stappers

Post by Geert Stappers
FWIW I'm formatting the patch so it be `git am`

} FWIW I'm formatting the patch so it can be `git am` processed
Is this the 'correct' way to do it?
I couldn't really find any information on how to contribute to dnsmasq.

Sending patches that "git apply-mail" can handle, brings often success.
I think it is because project owner doesn't need to spend
brain power on creating a commit message, pasting in authors name.

Post by Geert Stappers
It will have Andy's name, but no sign-off.

Is that something I need to do?

a commit message, am in no postition to sign-off with Andy's name.
Sending an email in his name already feels wrong ...
My previous message

Post by Geert Stappers
FWIW I'm formatting the patch so it can be `git am` processed
It will have Andy's name, but no sign-off.

|Yes, dnsmasq in nice software. I do use it, I do want to contribute.
|I do follow the mailinglist. I have seen a patch on the mailinglist.
|Also seen that it will take Simon Kelley more then needed effort
|to be applied to the git repository. So I announced the reformatting
|and use (abuse?) of Andy's name in the upcoming e-mail.

I know it's fun to come up with a patch to fix a supposed problem with
a widely-employed piece of software, but stop for a minute and think
about what you're attempting to "achieve".

If successful, you will add just another piece of bloat (that is
subject to error and will have to be tested) to dnsmasq to address a
problem that is not in fact dnsmasq's, but a misconfiguration problem
at the *user's* end.

Kurt has already told you how to fix this. It's a trivial fix to your
editor's configuration that you should be doing *anyway* (for reasons
that Kurt has already indicated). I find it quite amazing that one
can devote this much effort to "solve" a problem in dnsmasq that you
can (and should) fix on your end.

I have no idea what Simon's attitude to all this it, but *I* want to
be put on record as being in *complete* agreement with Kurt on how to
"fix" this (and that's not a patch to dnsmasq).

[Andy Hawkins]

Post by Will Parsons
I know it's fun to come up with a patch to fix a supposed problem with
a widely-employed piece of software, but stop for a minute and think
about what you're attempting to "achieve".
If successful, you will add just another piece of bloat (that is
subject to error and will have to be tested) to dnsmasq to address a
problem that is not in fact dnsmasq's, but a misconfiguration problem
at the *user's* end.

The code already exists in dnsmasq, it just doesn't work properly.

This is the block of code in question (around line 235 in src/inotify.c):

/* ignore emacs backups and dotfiles */
if (in->len == 0 ||
in->name[in->len - 1] == '~' ||
(in->name[0] == '#' && in->name[in->len - 1] == '#') ||
in->name[0] == '.')
continue;

What it's trying to do, is ignore any file whose last characeter is a '~',
or first and last characters are '#', or first character is '.'.

However, it's incorrectly using 'in->len', assuming this indicates the
length of the file name. However, it actually indicates the length of the
*buffer* containing the file name (which appears to be being allocated in
something like 16 byte chunks).

The patch is simply replacing 'in->len - 1' with 'strlen(in->name) - 1' (on
two lines) to correctly get the last character from the name, so it's hardly
adding 'bloat', it's merely fixing functionality that has already been
attempted but implemented incorrectly.

Andy

[Simon Kelley]

Post by Will Parsons

Post by Geert Stappers

Post by Geert Stappers
FWIW I'm formatting the patch so it be `git am`

} FWIW I'm formatting the patch so it can be `git am` processed
Is this the 'correct' way to do it?
I couldn't really find any information on how to contribute to dnsmasq.

Sending patches that "git apply-mail" can handle, brings often success.
I think it is because project owner doesn't need to spend
brain power on creating a commit message, pasting in authors name.

Post by Geert Stappers
It will have Andy's name, but no sign-off.

Is that something I need to do?

a commit message, am in no postition to sign-off with Andy's name.
Sending an email in his name already feels wrong ...
My previous message

Post by Geert Stappers
FWIW I'm formatting the patch so it can be `git am` processed
It will have Andy's name, but no sign-off.

|Yes, dnsmasq in nice software. I do use it, I do want to contribute.
|I do follow the mailinglist. I have seen a patch on the mailinglist.
|Also seen that it will take Simon Kelley more then needed effort
|to be applied to the git repository. So I announced the reformatting
|and use (abuse?) of Andy's name in the upcoming e-mail.

I know it's fun to come up with a patch to fix a supposed problem with
a widely-employed piece of software, but stop for a minute and think
about what you're attempting to "achieve".
If successful, you will add just another piece of bloat (that is
subject to error and will have to be tested) to dnsmasq to address a
problem that is not in fact dnsmasq's, but a misconfiguration problem
at the *user's* end.
Kurt has already told you how to fix this. It's a trivial fix to your
editor's configuration that you should be doing *anyway* (for reasons
that Kurt has already indicated). I find it quite amazing that one
can devote this much effort to "solve" a problem in dnsmasq that you
can (and should) fix on your end.
I have no idea what Simon's attitude to all this it, but *I* want to
be put on record as being in *complete* agreement with Kurt on how to
"fix" this (and that's not a patch to dnsmasq).

FWIW, Simon's attitude is that if the code to ignore common editor
droppings didn't already exist in dnsmasq, then we could argue about
whether it was a good idea. Since it does, and has done for a long time,
and isn't going to be removed (for backward compatibility reasons, if no
other) then having that argument is a bit pointless.

Andy appears to have found a bug, wherein this behaviour is clearly
intended to happen, and doesn't, because I misunderstood the inotify
API. Fixing that is a no-brainer.

I'm touched by all the effort that's gone into making a suitable patch
whilst I've been elsewhere. For the record, anything that I can feed to
"git apply" is fine. I don't care too much about making it really,
really easy to apply, since that makes me lazy, and I try to look at,
and think about, every patch that I apply.


Cheers,

Simon.

[Olaf Hering]

Post by Kurt H Maier
You should fix the editor; that behavior is dangerous for other reasons,
http://openwall.com/lists/oss-security/2017/11/27/2

This talks about apples, while Andy talks about oranges.
Fix "$dnsmasq" to process only files intended for "$dnsmasq".

Olaf

[Kurt H Maier]

Post by Olaf Hering
This talks about apples, while Andy talks about oranges.
Fix "$dnsmasq" to process only files intended for "$dnsmasq".

Fix "$editor" to edit files instead of littering in the place you put
files to indicate they're intended for "$dnsmasq".

khm