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

[Simon Kelley]

On 13/02/18 01:31, Will Parsons wrote:
> On Monday, 12 Feb 2018 10:06 AM -0500, Geert Stappers wrote:
>> On Mon, Feb 12, 2018 at 01:41:19PM -, Andy Hawkins wrote:
>>> In article <20180212104746.gb9...@gpm.stappers.nl>, Geert Stappers wrote:
 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.
>>
 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
 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.
> 
> 
> 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.


___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

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

[Andy Hawkins]

In article ,
   Will Parsons wrote:
> 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


___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

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

[Will Parsons]

On Monday, 12 Feb 2018 10:06 AM -0500, Geert Stappers wrote:
> On Mon, Feb 12, 2018 at 01:41:19PM -, Andy Hawkins wrote:
>> In article <20180212104746.gb9...@gpm.stappers.nl>, Geert Stappers wrote:
>> > 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.
>
>> > 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
>> > 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.


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).

-- 
Will


___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

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

[Andy Hawkins]

Hi,

In article <45676db9-d890-14a0-7743-f0340b7d1...@mail.com>,
   john doe wrote:
>> [andy@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
>
> 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


___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

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

[john doe]

On 2/11/2018 4:58 PM, Andy Hawkins wrote:

Hi,

In article ,
Andy Hawkins wrote:

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.

[andy@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


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


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
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss




--
John Doe

___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

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

[Andy Hawkins]

Hi,

In article ,
   Andy Hawkins wrote:
> 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.

[andy@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


___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

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

[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


On 11.02.2018 12:57, Andy Hawkins wrote:
> Hi,
>
> In article ,
>Andy Hawkins wrote:
>> 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~
> 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
>
>
> ___
> Dnsmasq-discuss mailing list
> Dnsmasq-discuss@lists.thekelleys.org.uk
> http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss


___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

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

[Andy Hawkins]

Hi,

In article ,
   Andy Hawkins wrote:
> 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~

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


___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

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

[Geert Stappers]

On Thu, Feb 08, 2018 at 09:11:43PM -, Andy Hawkins wrote:
> In article <20180208164432.GA97242@wopr>, Kurt H Maier wrote:
> > 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
> 
> 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
-- 
Leven en laten leven

___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

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

[Kurt H Maier]

On Fri, Feb 09, 2018 at 05:33:19PM +0100, Olaf Hering wrote:
> 
> 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

___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

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

[Olaf Hering]

On Thu, Feb 08, Kurt H Maier wrote:

> 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

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

Olaf


signature.asc
Description: PGP signature
___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

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

[Andy Hawkins]

Hi,

In article <20180208164432.GA97242@wopr>,
   Kurt H Maier wrote:
> 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

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


___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

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

[Kurt H Maier]

On Thu, Feb 08, 2018 at 03:18:00PM -, Andy Hawkins wrote:
>
> 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

___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss