Merge ~mirespace/ubuntu/+source/dnsmasq:lp2015562-jammy-segfault-nullAAAAconfig into ubuntu/+source/dnsmasq:ubuntu/jammy-devel
- Git
- lp:~mirespace/ubuntu/+source/dnsmasq
- lp2015562-jammy-segfault-nullAAAAconfig
- Merge into ubuntu/jammy-devel
Status: | Merged |
---|---|
Approved by: | git-ubuntu bot |
Approved revision: | not available |
Merged at revision: | e23dc7592da34fce544249756bd2b17a20900ede |
Proposed branch: | ~mirespace/ubuntu/+source/dnsmasq:lp2015562-jammy-segfault-nullAAAAconfig |
Merge into: | ubuntu/+source/dnsmasq:ubuntu/jammy-devel |
Diff against target: |
265 lines (+89/-72) 3 files modified
debian/changelog (+14/-0) src/dnsmasq.h (+17/-17) src/domain-match.c (+58/-55) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
git-ubuntu bot | Approve | ||
Sergio Durigan Junior (community) | Approve | ||
Canonical Server Reporter | Pending | ||
Review via email:
|
Commit message
Description of the change
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Miriam España Acebal (mirespace) wrote : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Miriam España Acebal (mirespace) wrote (last edit ): | # |
Steps to reproduce are (thanks Sergio for your help on this!):
#0.Prepare a VM or Container. i.e:
# lxc launch ubuntu-daily:jammy Jdnsmasq
#1. Install dnsmasq
# apt update && apt upgrade -y
# apt install y dnsmasq
#2. Disable systemd-resolved service and enabling resolution through dnsmasq, configuring DNS servers through it.
# systemctl disable --now systemd-
# rm -f /etc/resolv.conf
# cat > /etc/resolv.conf << __EOF__
nameserver 127.0.0.1
__EOF__
# echo "server 8.8.8.8" >> /etc/dnsmasq.conf (or edit the file to add it if you prefer)
# (Optional) echo "log-queries" >> /etc/dnsmasq.conf
# (optional) echo "log-debug" >> /etc/dnsmasq.conf
# systemctl start dnsmasq.service
3. Copy netflix-nov6.conf into /etc/dnsmasq.d/
# cat > /etc/dnsmasq.
# Null AAAA response on these domains
server=
address=
server=
address=
server=
address=
__EOF__
#4. Restart/reload dnsmasq
# systemctl restart dnsmasq
#5. Verify that dnsmasq resolves domains correctly:
root@Jdnsmasq:~# dig +short -tA ubuntu.com @127.0.0.1
185.125.190.21
185.125.190.20
185.125.190.29
root@Jdnsmasq:~# dig +short -tAAAA ubuntu.com @127.0.0.1
2620:2d:4000:1::28
2620:2d:4000:1::26
2620:2d:4000:1::27
#6. Perform a type65 / HTTPS recordtype query for netflix.com towards the dnsmasq server once or twice:
root@Jdnsmasq:~# dig +short -tTYPE65 netflix.com @127.0.0.1
root@Jdnsmasq:~# dig +short -tTYPE65 netflix.com @127.0.0.1
;; communications error to 127.0.0.1#53: timed out
;; communications error to 127.0.0.1#53: connection refused
;; communications error to 127.0.0.1#53: connection refused
;; no servers could be reached
#7. Check logs to verify segfault:
# journalctl -u dnsmasq
Apr 27 11:22:52 Jdnsmasq systemd[1]: Started dnsmasq - A lightweight DHCP and caching DNS server.
Apr 27 11:22:53 Jdnsmasq dnsmasq[111585]: query[type=65] netflix.com from 127.0.0.1
Apr 27 11:22:53 Jdnsmasq dnsmasq[111585]: config error is REFUSED (EDE: network error)
Apr 27 11:22:54 Jdnsmasq dnsmasq[111585]: query[type=65] netflix.com from 127.0.0.1
Apr 27 11:22:54 Jdnsmasq systemd[1]: dnsmasq.service: Main process exited, code=dumped, status=11/SEGV
Apr 27 11:22:54 Jdnsmasq systemd[1]: dnsmasq.service: Failed with result 'core-dump'.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Sergio Durigan Junior (sergiodj) wrote : | # |
Thanks for the MP, Miriam.
This segmentation fault is a bit tricky to reproduce. For some reason, I was not able to make dnsmasq crash just by following the instructions from the report (even though I remember being able to see the crash before!), so I went on a parallel investigation to try and understand better what's going on.
Long story short, I found the upstream bug report here:
https://<email address hidden>
And I also found that in order to reproduce the bug reliably I had to add yet another "server/address" pair to the configuration file provided by the reporter. I ended up with this:
server=
address=
server=
address=
server=
address=
server=
address=
Now I see the crash happen reliably.
OK, now let's talk about the proposed changes.
At first I was concerned about the redefinitions on dnsmasq.h, because it seemed like they could introduce some ABI breakage, but after looking deeper I came to believe that they are fine. They're internal to dnsmasq and it doesn't export the header anywhere, so it should be safe.
Now, what makes me uncomfortable are the changes introduced in the domain-match.c file. They lean toward the non-trivial side, and the code itself is not easy to follow. I would like to see a deeper verification to make sure that the domain matching functionality is not affected by the change being proposed here. Maybe a few simple testcases can do the trick and increase our confidence.
Unfortunately, I don't have enough time right now to do this verification. If you feel like doing it, that would be great (and I can also review the tests/results if needed). Otherwise, I think I will be a bit less busy in two weeks and I can try to help with this task.
As for whether this MP is "SRU material", I really can't say. On the one hand, the changes can be considered non-trivial, but that alone is not enough of a reason to give up. On the other hand, we were able to confirm that the backported patch fixes the issue. On top of that, I clone upstream's repository and checked the history of domain-match.c. There are fixes that seem important and that came after the patch you're backporting, but they don't seem to relate to this bug. This gives us a bit more confidence in the fix, since upstream has been carrying it for a while now without any apparent regressions. So, IMHO, we should proceed with the SRU once we have those test results and see how it will be received.
Before I forget, there are some minor nits that I found in the Test Plan:
- "apt install y dnsmasq" -- This needs a dash before the "y"
- echo "server 8.8.8.8" >> /etc/dnsmasq.conf -- This doesn't work; it should be "server=8.8.8.8", otherwise the service won't start.
- I believe you can use the example I posted above for the /etc/dnsmasq.
- "dig +short -tTYPE65 netflix.com @127.0.0.1" -- No need to use -tTYPE65 here. A simple "dig A test.netflix.net @127.0.0.1" should suffice.
Thanks a lot!
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Miriam España Acebal (mirespace) wrote (last edit ): | # |
Hi again,
I'm working on a dep8 test for checking domain matching functionality (based on the ones I've found that were submitted to Debian at https:/
autopkgtest: WARNING: Test dependencies are unsatisfiable - calling apt install on test deps directly for further data about failing dependencies in test logs.
/bin/sh: 1: bind9-utils: not found
autopkgtest [12:46:20]: - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
(but bind9-utils can be installed if I log in to the virtual machine)
Thanks Sergio for your super review and for catching the errors on the steps to reproduce!!!
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Sergio Durigan Junior (sergiodj) wrote : | # |
> Hi again,
Hey Miriam :-),
> I'm working on a dep8 test for checking domain matching functionality (based
> on the ones I've found that were submitted to Debian at
> https:/
> MM... without much luck since I'm dealing with test dependencies (I'm trying
> to figure out why):
>
> autopkgtest: WARNING: Test dependencies are unsatisfiable - calling apt
> install on test deps directly for further data about failing dependencies in
> test logs.
> /bin/sh: 1: bind9-utils: not found
> autopkgtest [12:46:20]: - - - - - - - - - - - - - - - - - - - - - - - - - - -
> - -
>
> (but bind9-utils can be installed if I log in to the virtual machine)
Hm, that's strange. The log is also a bit weird: it seems like the script is trying to invoke a binary called "bind9-utils", which doesn't exist. Can you provide more details, please? The dep8 script and the contents of d/t/control would be helpful.
> Thanks Sergio for your super review and for catching the errors on the steps
> to reproduce!!!
You're welcome!
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Miriam España Acebal (mirespace) wrote : | # |
Hi Sergio, I finally made the tests work!
autopkgtest -U -s --setup-
Results:
autopkgtest [17:31:48]: @@@@@@@
smoke-test PASS
dhcp-test PASS
udp-packet-
These tests are based on the proposal made to Debian at https:/
I doubted extracting the dnsmasq's configuration and interfaces setup for making it common to all the tests (they are pretty much the same), but after consideration, I thought that the removal of a test (if needed) would be easier to do if I left it as is.
To be honest, what was the tests proposal reporter's intention with the size udp test is unknown to me (he said it was unfinished).
Interesting what I learned about dig searches for ipv6: it launches some simultaneously, so you can get a timeout warning message (not on stderr). Then, noisy output could be gotten ( the grep -v can also be seen in bind9 testsuite).
Also, I included a setup-commands file with the intention to automatize it (I read that it should be installed in /usr/share/
Thankyou!
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Sergio Durigan Junior (sergiodj) wrote : | # |
Hi Miriam,
I had some time to re-read our discussion and take a look at your branch. Thanks for the work you've done in it.
The dep8 tests you're proposing in this MP look like a good addition to the package (although I haven't looked deep into them), but as I mentioned to you earlier I believe that they more suitable for our development release, not for a Jammy SRU. The tests look complex and end up exercising several features that are not being touched by this proposed change, so in my mind they're orthogonal to the problem at hand.
Having said that, I also believe that it should be possible to use the SRU's Test Plan as a base to write a simple script to test the domain matching feature. You can think of a few possible scenarios to test:
- The regular "address=
- The situation that causes the bug being fixed by this MP (i.e., several "server"/"address" directives in the file).
- Maybe another combination of "server"/"address", just in case.
In order to write such a test, you will need the "needs-root" restriction because you'll have to make sure systemd-resolved is disabled/stopped and that dnsmasq is started. You should be able to do everything inside a shell script.
This should be much simpler than dealing with the dep8 scripts you backported from Debian, and enough to provide a bit more confidence in the upstream patch.
Hopefully this is helpful. Please let me know if you need anything else from my side; I'll make sure to reply in a more timely manner :-).
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Miriam España Acebal (mirespace) wrote (last edit ): | # |
Hi Sergio,
thanks for the talks and the on-live session we shared for this.
I updated the bug's description with the SRU paperwork. Please let me know what do you think about it, when it fits you well.
I removed the DEP8 tests and I rebuilt also the package without it (in the same ppa). Maybe you will need a debdiff from actual Jammy version to the proposed one in the ppa? I see the diff in the ppa is only for the versions inside it sadly .
Thanks!
P.S.: A thought: could that be a functionality which can be proposed to Launchpad team (to have also the debdiff against the latest version in the series, not only refered to the incremental build inside the ppa) ?)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Sergio Durigan Junior (sergiodj) wrote : | # |
Hi Miriam!
Thank you very much for addressing my comments and updating the SRU text. It's looking great now. I double checked here and I could easily reproduce the problem by following the "Test Plan" section. I could also verify that the bug is fixed with the version of the package present in your PPA.
Thanks also for the "Other Info" section; that's exactly what I wanted :-). It is still possible that the SRU team will request more information from you (like more details about the change, or a more comprehensive testcase), but let's not get ahead of ourselves and just wait for their feedback.
I am sponsoring this upload. Thanks again for the patience and perseverance!
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
git-ubuntu bot (git-ubuntu-bot) wrote : | # |
Approvers: sergiodj, mirespace
Uploaders: sergiodj
MP auto-approved
Preview Diff
1 | diff --git a/debian/changelog b/debian/changelog |
2 | index b1d883a..23e6ba8 100644 |
3 | --- a/debian/changelog |
4 | +++ b/debian/changelog |
5 | @@ -1,3 +1,17 @@ |
6 | +dnsmasq (2.86-1.1ubuntu0.4) jammy; urgency=medium |
7 | + |
8 | + * src/dnsmasq.h, src/domain-match.c: Fix confusion when using resolvconf |
9 | + servers (combining server|address for a domain), resulting in the struct |
10 | + server datastructure for server=/domain/# getting passed to |
11 | + forward_query(), rapidly followed by a SEGV. This fix makes |
12 | + server=/domain/# a fully fledged member of the priority list. |
13 | + The code added here is a cherry pick released in upstream version |
14 | + 2.87, originating at |
15 | + https://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=commit;h=de372d69 |
16 | + (LP: #2015562) |
17 | + |
18 | + -- Miriam España Acebal <miriam.espana@canonical.com> Thu, 20 Apr 2023 11:00:27 +0200 |
19 | + |
20 | dnsmasq (2.86-1.1ubuntu0.3) jammy-security; urgency=medium |
21 | |
22 | * SECURITY UPDATE: IP fragmentation |
23 | diff --git a/src/dnsmasq.h b/src/dnsmasq.h |
24 | index 8674823..08f2785 100644 |
25 | --- a/src/dnsmasq.h |
26 | +++ b/src/dnsmasq.h |
27 | @@ -530,23 +530,23 @@ union mysockaddr { |
28 | |
29 | |
30 | /* The actual values here matter, since we sort on them to get records in the order |
31 | - IPv6 addr, IPv4 addr, all zero return, no-data return, send upstream. */ |
32 | -#define SERV_LITERAL_ADDRESS 1 /* addr is the answer, or NoDATA is the answer, depending on the next three flags */ |
33 | -#define SERV_ALL_ZEROS 2 /* return all zeros for A and AAAA */ |
34 | -#define SERV_4ADDR 4 /* addr is IPv4 */ |
35 | -#define SERV_6ADDR 8 /* addr is IPv6 */ |
36 | -#define SERV_HAS_SOURCE 16 /* source address defined */ |
37 | -#define SERV_FOR_NODOTS 32 /* server for names with no domain part only */ |
38 | -#define SERV_WARNED_RECURSIVE 64 /* avoid warning spam */ |
39 | -#define SERV_FROM_DBUS 128 /* 1 if source is DBus */ |
40 | -#define SERV_MARK 256 /* for mark-and-delete and log code */ |
41 | -#define SERV_WILDCARD 512 /* domain has leading '*' */ |
42 | -#define SERV_USE_RESOLV 1024 /* forward this domain in the normal way */ |
43 | -#define SERV_FROM_RESOLV 2048 /* 1 for servers from resolv, 0 for command line. */ |
44 | -#define SERV_FROM_FILE 4096 /* read from --servers-file */ |
45 | -#define SERV_LOOP 8192 /* server causes forwarding loop */ |
46 | -#define SERV_DO_DNSSEC 16384 /* Validate DNSSEC when using this server */ |
47 | -#define SERV_GOT_TCP 32768 /* Got some data from the TCP connection */ |
48 | + IPv6 addr, IPv4 addr, all zero return, resolvconf servers, upstream server, no-data return */ |
49 | +#define SERV_LITERAL_ADDRESS 1 /* addr is the answer, or NoDATA is the answer, depending on the next four flags */ |
50 | +#define SERV_USE_RESOLV 2 /* forward this domain in the normal way */ |
51 | +#define SERV_ALL_ZEROS 4 /* return all zeros for A and AAAA */ |
52 | +#define SERV_4ADDR 8 /* addr is IPv4 */ |
53 | +#define SERV_6ADDR 16 /* addr is IPv6 */ |
54 | +#define SERV_HAS_SOURCE 32 /* source address defined */ |
55 | +#define SERV_FOR_NODOTS 64 /* server for names with no domain part only */ |
56 | +#define SERV_WARNED_RECURSIVE 128 /* avoid warning spam */ |
57 | +#define SERV_FROM_DBUS 256 /* 1 if source is DBus */ |
58 | +#define SERV_MARK 512 /* for mark-and-delete and log code */ |
59 | +#define SERV_WILDCARD 1024 /* domain has leading '*' */ |
60 | +#define SERV_FROM_RESOLV 2048 /* 1 for servers from resolv, 0 for command line. */ |
61 | +#define SERV_FROM_FILE 4096 /* read from --servers-file */ |
62 | +#define SERV_LOOP 8192 /* server causes forwarding loop */ |
63 | +#define SERV_DO_DNSSEC 16384 /* Validate DNSSEC when using this server */ |
64 | +#define SERV_GOT_TCP 32768 /* Got some data from the TCP connection */ |
65 | |
66 | struct serverfd { |
67 | int fd; |
68 | diff --git a/src/domain-match.c b/src/domain-match.c |
69 | index f8e4796..9ddb638 100644 |
70 | --- a/src/domain-match.c |
71 | +++ b/src/domain-match.c |
72 | @@ -207,16 +207,16 @@ int lookup_domain(char *domain, int flags, int *lowout, int *highout) |
73 | } |
74 | } |
75 | |
76 | - if (found) |
77 | + if (found && filter_servers(try, flags, &nlow, &nhigh)) |
78 | + /* We have a match, but it may only be (say) an IPv6 address, and |
79 | + if the query wasn't for an AAAA record, it's no good, and we need |
80 | + to continue generalising */ |
81 | { |
82 | /* We've matched a setting which says to use servers without a domain. |
83 | Continue the search with empty query */ |
84 | - if (daemon->serverarray[try]->flags & SERV_USE_RESOLV) |
85 | + if (daemon->serverarray[nlow]->flags & SERV_USE_RESOLV) |
86 | crop_query = qlen; |
87 | - else if (filter_servers(try, flags, &nlow, &nhigh)) |
88 | - /* We have a match, but it may only be (say) an IPv6 address, and |
89 | - if the query wasn't for an AAAA record, it's no good, and we need |
90 | - to continue generalising */ |
91 | + else |
92 | break; |
93 | } |
94 | } |
95 | @@ -273,7 +273,7 @@ int filter_servers(int seed, int flags, int *lowout, int *highout) |
96 | nlow--; |
97 | |
98 | while (nhigh < daemon->serverarraysz-1 && order_servers(daemon->serverarray[nhigh], daemon->serverarray[nhigh+1]) == 0) |
99 | - nhigh++; |
100 | + nhigh++; |
101 | |
102 | nhigh++; |
103 | |
104 | @@ -293,10 +293,10 @@ int filter_servers(int seed, int flags, int *lowout, int *highout) |
105 | else |
106 | { |
107 | /* Now the servers are on order between low and high, in the order |
108 | - IPv6 addr, IPv4 addr, return zero for both, send upstream, no-data return. |
109 | + IPv6 addr, IPv4 addr, return zero for both, resolvconf servers, send upstream, no-data return. |
110 | |
111 | See which of those match our query in that priority order and narrow (low, high) */ |
112 | - |
113 | + |
114 | for (i = nlow; i < nhigh && (daemon->serverarray[i]->flags & SERV_6ADDR); i++); |
115 | |
116 | if (i != nlow && (flags & F_IPV6)) |
117 | @@ -321,32 +321,40 @@ int filter_servers(int seed, int flags, int *lowout, int *highout) |
118 | { |
119 | nlow = i; |
120 | |
121 | - /* now look for a server */ |
122 | - for (i = nlow; i < nhigh && !(daemon->serverarray[i]->flags & SERV_LITERAL_ADDRESS); i++); |
123 | - |
124 | + /* Short to resolv.conf servers */ |
125 | + for (i = nlow; i < nhigh && (daemon->serverarray[i]->flags & SERV_USE_RESOLV); i++); |
126 | + |
127 | if (i != nlow) |
128 | - { |
129 | - /* If we want a server that can do DNSSEC, and this one can't, |
130 | - return nothing, similarly if were looking only for a server |
131 | - for a particular domain. */ |
132 | - if ((flags & F_DNSSECOK) && !(daemon->serverarray[nlow]->flags & SERV_DO_DNSSEC)) |
133 | - nlow = nhigh; |
134 | - else if ((flags & F_DOMAINSRV) && daemon->serverarray[nlow]->domain_len == 0) |
135 | - nlow = nhigh; |
136 | - else |
137 | - nhigh = i; |
138 | - } |
139 | + nhigh = i; |
140 | else |
141 | { |
142 | - /* --local=/domain/, only return if we don't need a server. */ |
143 | - if (flags & (F_DNSSECOK | F_DOMAINSRV | F_SERVER)) |
144 | - nhigh = i; |
145 | + /* now look for a server */ |
146 | + for (i = nlow; i < nhigh && !(daemon->serverarray[i]->flags & SERV_LITERAL_ADDRESS); i++); |
147 | + |
148 | + if (i != nlow) |
149 | + { |
150 | + /* If we want a server that can do DNSSEC, and this one can't, |
151 | + return nothing, similarly if were looking only for a server |
152 | + for a particular domain. */ |
153 | + if ((flags & F_DNSSECOK) && !(daemon->serverarray[nlow]->flags & SERV_DO_DNSSEC)) |
154 | + nlow = nhigh; |
155 | + else if ((flags & F_DOMAINSRV) && daemon->serverarray[nlow]->domain_len == 0) |
156 | + nlow = nhigh; |
157 | + else |
158 | + nhigh = i; |
159 | + } |
160 | + else |
161 | + { |
162 | + /* --local=/domain/, only return if we don't need a server. */ |
163 | + if (flags & (F_DNSSECOK | F_DOMAINSRV | F_SERVER)) |
164 | + nhigh = i; |
165 | + } |
166 | } |
167 | } |
168 | } |
169 | } |
170 | } |
171 | - |
172 | + |
173 | *lowout = nlow; |
174 | *highout = nhigh; |
175 | |
176 | @@ -520,10 +528,10 @@ static int order_qsort(const void *a, const void *b) |
177 | /* Sort all literal NODATA and local IPV4 or IPV6 responses together, |
178 | in a very specific order. We flip the SERV_LITERAL_ADDRESS bit |
179 | so the order is IPv6 literal, IPv4 literal, all-zero literal, |
180 | - upstream server, NXDOMAIN literal. */ |
181 | + unqualified servers, upstream server, NXDOMAIN literal. */ |
182 | if (rc == 0) |
183 | - rc = ((s2->flags & (SERV_LITERAL_ADDRESS | SERV_4ADDR | SERV_6ADDR | SERV_ALL_ZEROS)) ^ SERV_LITERAL_ADDRESS) - |
184 | - ((s1->flags & (SERV_LITERAL_ADDRESS | SERV_4ADDR | SERV_6ADDR | SERV_ALL_ZEROS)) ^ SERV_LITERAL_ADDRESS); |
185 | + rc = ((s2->flags & (SERV_LITERAL_ADDRESS | SERV_4ADDR | SERV_6ADDR | SERV_USE_RESOLV | SERV_ALL_ZEROS)) ^ SERV_LITERAL_ADDRESS) - |
186 | + ((s1->flags & (SERV_LITERAL_ADDRESS | SERV_4ADDR | SERV_6ADDR | SERV_USE_RESOLV | SERV_ALL_ZEROS)) ^ SERV_LITERAL_ADDRESS); |
187 | |
188 | /* Finally, order by appearance in /etc/resolv.conf etc, for --strict-order */ |
189 | if (rc == 0) |
190 | @@ -630,7 +638,7 @@ int add_update_server(int flags, |
191 | { |
192 | size_t size; |
193 | |
194 | - if (flags & SERV_LITERAL_ADDRESS) |
195 | + if (flags & SERV_IS_LOCAL) |
196 | { |
197 | if (flags & SERV_6ADDR) |
198 | size = sizeof(struct serv_addr6); |
199 | @@ -649,10 +657,19 @@ int add_update_server(int flags, |
200 | { |
201 | serv->next = daemon->local_domains; |
202 | daemon->local_domains = serv; |
203 | + |
204 | + if (flags & SERV_4ADDR) |
205 | + ((struct serv_addr4*)serv)->addr = local_addr->addr4; |
206 | + |
207 | + if (flags & SERV_6ADDR) |
208 | + ((struct serv_addr6*)serv)->addr = local_addr->addr6; |
209 | } |
210 | else |
211 | { |
212 | struct server *s; |
213 | + |
214 | + memset(serv, 0, sizeof(struct server)); |
215 | + |
216 | /* Add to the end of the chain, for order */ |
217 | if (!daemon->servers) |
218 | daemon->servers = serv; |
219 | @@ -662,37 +679,23 @@ int add_update_server(int flags, |
220 | s->next = serv; |
221 | } |
222 | |
223 | - serv->next = NULL; |
224 | +#ifdef HAVE_LOOP |
225 | + serv->uid = rand32(); |
226 | +#endif |
227 | + |
228 | + if (interface) |
229 | + safe_strncpy(serv->interface, interface, sizeof(serv->interface)); |
230 | + if (addr) |
231 | + serv->addr = *addr; |
232 | + if (source_addr) |
233 | + serv->source_addr = *source_addr; |
234 | } |
235 | } |
236 | |
237 | - if (!(flags & SERV_IS_LOCAL)) |
238 | - memset(serv, 0, sizeof(struct server)); |
239 | - |
240 | serv->flags = flags; |
241 | serv->domain = alloc_domain; |
242 | serv->domain_len = strlen(alloc_domain); |
243 | |
244 | - if (flags & SERV_4ADDR) |
245 | - ((struct serv_addr4*)serv)->addr = local_addr->addr4; |
246 | - |
247 | - if (flags & SERV_6ADDR) |
248 | - ((struct serv_addr6*)serv)->addr = local_addr->addr6; |
249 | - |
250 | - if (!(flags & SERV_IS_LOCAL)) |
251 | - { |
252 | -#ifdef HAVE_LOOP |
253 | - serv->uid = rand32(); |
254 | -#endif |
255 | - |
256 | - if (interface) |
257 | - safe_strncpy(serv->interface, interface, sizeof(serv->interface)); |
258 | - if (addr) |
259 | - serv->addr = *addr; |
260 | - if (source_addr) |
261 | - serv->source_addr = *source_addr; |
262 | - } |
263 | - |
264 | return 1; |
265 | } |
266 |
Hi team!
As discussed in the last Bug Housekeeping meeting, the upstream fix proposed by the user effectively makes the issue goes away, but the code is so much refactoring work maybe not suitable for an SRU as commented by Bryce initially (thanks!).
Package (tested that it fixes the issue) is here: dnsmasq- lp2015562 /launchpad. net/~mirespace/ +archive/ ubuntu/ dnsmasq- lp2015562
ppa:mirespace/
https:/
The package is native, so no patches are being applied, and the changes have been cherry-picked directly.
The aim of opening this MP is to debate if we can narrow the bits to make it suitable/candidate for SRU process. I would to say that, for me, only changing a global definition value and affecting with it all the rest of the variables makes me think of a big change in behaviour, but I have to admit I didn't trace it completely on all the dnsmasq code.
Another thing that worries me is that I couldn't reproduce the issue on Kinetic, although the upstream version is the same. So I checked the "ubuntu differences" between the versions, and there are no such differences: https:/ /paste. ubuntu. com/p/S39n5Yy9g T/ So... is really the problem on dnsmasq??? Maybe not.
Due to this uncertainty, I didn't prepare the SRU paperwork, but I will do it if we consider the fix OK (or we get a subset of the fix that can work).