| Message ID | 20220728034508.15180-2-selva.nair@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,1/2] Do not skip ERROR:/SUCCESS: response from management interface | expand |
Acked-by: Gert Doering <gert@greenie.muc.de> I cannot test this (beyond "compile", but that is trivial) but the description in https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24738.html makes sense, so allowing "a limited amount" of recursion plus actually logging when this is hit should make management more robust against overlapping timing issues aka "race conditions". Your patch has been applied to the master branch. commit 4dfd592ff1ee18aa4859264c8a341dfa1a291988 Author: Selva Nair Date: Wed Jul 27 23:45:08 2022 -0400 Allow a few levels of recursion in virtual_output_callback() Signed-off-by: Selva Nair <selva.nair@gmail.com> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20220728034508.15180-2-selva.nair@gmail.com> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24751.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
Hi, On Tue, Aug 2, 2022 at 8:02 AM Gert Doering <gert@greenie.muc.de> wrote: > Acked-by: Gert Doering <gert@greenie.muc.de> > > I cannot test this (beyond "compile", but that is trivial) but the > description in > > > https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24738.html > > makes sense, so allowing "a limited amount" of recursion plus actually > logging when this is hit should make management more robust against > overlapping timing issues aka "race conditions". > > Your patch has been applied to the master branch. > Can we also apply this to 2.5? I see this trigerred easily with the new persistent connection / PLAP support. I haven't figured the exact reason but hitting reconnect or disconnect at certain instances triggers this -- like when the connection is almost complete and the daemon is doing slow-ish netsh commands. The same code-path is not traversed when interactive service is in use and it seems hitting the "race condition" is much harder in that case. Otherwise we have to limit these new GUI features to 2.6 which would require a new branch in the GUI and some changes to the build process. In some sense this is a "bug fix" though the fix itself is a band-aid as opposed to implementing a command queue to save these write actions instead of dropping them. Selva <div dir="ltr"><div>Hi,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Aug 2, 2022 at 8:02 AM Gert Doering <<a href="mailto:gert@greenie.muc.de">gert@greenie.muc.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Acked-by: Gert Doering <<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>><br> <br> I cannot test this (beyond "compile", but that is trivial) but the<br> description in <br> <br> <a href="https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24738.html" rel="noreferrer" target="_blank">https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24738.html</a><br> <br> makes sense, so allowing "a limited amount" of recursion plus actually<br> logging when this is hit should make management more robust against <br> overlapping timing issues aka "race conditions".<br> <br> Your patch has been applied to the master branch.<br></blockquote><div><br></div><div>Can we also apply this to 2.5? I see this trigerred easily with the new persistent connection / PLAP support. I haven't figured the exact reason but hitting reconnect or disconnect at certain instances triggers this -- like when the connection is almost complete and the daemon is doing slow-ish netsh commands. The same code-path is not traversed when interactive service is in use and it seems hitting the "race condition" is much harder in that case.</div><div><br></div><div>Otherwise we have to limit these new GUI features to 2.6 which would require a new branch in the GUI and some changes to the build process.</div><div><br></div><div>In some sense this is a "bug fix" though the fix itself is a band-aid as opposed to implementing a command queue to save these write actions instead of dropping them.</div><div><br></div><div>Selva</div></div></div>
In case this request was lost, here goes again. Can we have this cherry-picked into 2.5 before the next release? Selva On Thu, Aug 11, 2022 at 4:03 PM Selva Nair <selva.nair@gmail.com> wrote: > Hi, > > On Tue, Aug 2, 2022 at 8:02 AM Gert Doering <gert@greenie.muc.de> wrote: > >> Acked-by: Gert Doering <gert@greenie.muc.de> >> >> I cannot test this (beyond "compile", but that is trivial) but the >> description in >> >> >> https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24738.html >> >> makes sense, so allowing "a limited amount" of recursion plus actually >> logging when this is hit should make management more robust against >> overlapping timing issues aka "race conditions". >> >> Your patch has been applied to the master branch. >> > > Can we also apply this to 2.5? I see this trigerred easily with the new > persistent connection / PLAP support. I haven't figured the exact reason > but hitting reconnect or disconnect at certain instances triggers this -- > like when the connection is almost complete and the daemon is doing > slow-ish netsh commands. The same code-path is not traversed when > interactive service is in use and it seems hitting the "race condition" is > much harder in that case. > > Otherwise we have to limit these new GUI features to 2.6 which would > require a new branch in the GUI and some changes to the build process. > > In some sense this is a "bug fix" though the fix itself is a band-aid as > opposed to implementing a command queue to save these write actions instead > of dropping them. > > Selva >
Hi, On Mon, Aug 22, 2022 at 10:43:39PM -0400, Selva Nair wrote: > In case this request was lost, here goes again. Can we have this > cherry-picked into 2.5 before the next release? Oops. It got lost, was rediscovered last week, and got lost again. Sorry, and thanks for the reminder. gert
Hi, On Tue, Aug 02, 2022 at 02:02:39PM +0200, Gert Doering wrote: > Your patch has been applied to the master branch. > > commit 4dfd592ff1ee18aa4859264c8a341dfa1a291988 ... and after some reminding, to release/2.5 as well. commit 468f53ae3bfcfca6563acbf648e7a0bc6f94b038 (HEAD -> release/2.5) Author: Selva Nair <selva.nair@gmail.com> Date: Wed Jul 27 23:45:08 2022 -0400 Allow a few levels of recursion in virtual_output_callback() ... (cherry picked from commit 4dfd592ff1ee18aa4859264c8a341dfa1a291988) Since this code is virtually unchanged "since ever", it cherrypicked smoothly, diff looks good, and a small client-side test run worked just fine (same caveat as for the "master" patch, I have nothing which triggers this problem today). gert
diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c index 27aa49a8..5670e594 100644 --- a/src/openvpn/manage.c +++ b/src/openvpn/manage.c @@ -312,8 +312,7 @@ virtual_output_callback_func(void *arg, const unsigned int flags, const char *st #define AF_DID_PUSH (1<<0) #define AF_DID_RESET (1<<1) - - if (!recursive_level) /* don't allow recursion */ + if (recursive_level < 5) /* limit recursion */ { struct gc_arena gc = gc_new(); struct log_entry e; @@ -380,6 +379,12 @@ virtual_output_callback_func(void *arg, const unsigned int flags, const char *st --recursive_level; } + else + { + /* cannot use msg here */ + printf("virtual_output: message to management interface " + "dropped due to recursion: <%s>\n", str); + } } /*