864248 - Netd BandwidthController segmentation fault

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Albert [:albert]]

In function  BandwidthController::setCostlyAlert of netd/BandwidthController.cpp file, line 885:

 asprintf(&alertQuotaCmd, ALERT_IPT_TEMPLATE, "-I", chainNameAndPos, bytes, alertName, alertName);

Having that: const char BandwidthController::ALERT_IPT_TEMPLATE[] = "%s %s
%s -m quota2 ! --quota %lld --name %s";
the asprintf throws a segmentation fault because bytes is long instead of
string.

Comment 1

[Albert [:albert]]

Attachment: Fix segmentation fault according to the needed rule of iptable

Comment 2

[Michael Vines [:m1] [:evilmachines]]

Comment on attachment 740229 [details] [diff] [review]
Fix segmentation fault according to the needed rule of iptable

Review of attachment 740229 [details] [diff] [review]:
-----------------------------------------------------------------

::: BandwidthController.cpp
@@ +48,4 @@
>  #include "oem_iptables_hook.h"
>  
>  /* Alphabetical */
> +const char BandwidthController::ALERT_IPT_TEMPLATE[] = "%s %s -m quota2 ! --quota %lld --name %s";

I see ALERT_IPT_TEMPLATE used in other asprintf()s in this file, so I think that removing the 3rd %s will break them.   Would an alternate solution be to add an empty string, "", to the argument list on line 886 between the |chainNameAndPos| and |bytes| arguments?

Comment 3

[Albert [:albert]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #2)
> Comment on attachment 740229 [details] [diff] [review]
> Fix segmentation fault according to the needed rule of iptable
> 
> Review of attachment 740229 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: BandwidthController.cpp
> @@ +48,4 @@
> >  #include "oem_iptables_hook.h"
> >  
> >  /* Alphabetical */
> > +const char BandwidthController::ALERT_IPT_TEMPLATE[] = "%s %s -m quota2 ! --quota %lld --name %s";
> 
> I see ALERT_IPT_TEMPLATE used in other asprintf()s in this file, so I think
> that removing the 3rd %s will break them.   Would an alternate solution be
> to add an empty string, "", to the argument list on line 886 between the
> |chainNameAndPos| and |bytes| arguments?

Add an empty string "" does not solve the problem because it causes a double space and later the string is parsed by blank spaces to split command and arguments. As a result, command exec fails.

To solve it I placed an empty string to the first parameter and then remove the first space.

Comment 4

[Albert [:albert]]

Attachment: Fix segmentation fault according to the needed rule of iptable

Comment 5

[Albert [:albert]]

Attachment: Fix segmentation fault according to the needed rule of iptable

memmove index was wrong in previous patch

Comment 6

[Marcelino Veiga Tuimil [:sonmarce]]

All bugs blocking bug 858005 are landed but this one, so we need this one to be landed in a branch, as it is required for Firefox OX 1.2. Or maybe you already have a new version of netd for Firefox OS 1.2?

Comment 7

[Michael Vines [:m1] [:evilmachines]]

Thanks for the reminder.  I've landed this in our ICS tree and should appear on CAF early next week at https://www.codeaurora.org/cgit/quic/la/platform/system/netd/log/?h=b2g_ics_1.2 

It looks like JB MR2 netd is not affected.

Comment 8

[Albert [:albert]]

https://www.codeaurora.org/cgit/quic/la/platform/system/netd/log/?h=b2g_ics_1.2 returns Invalid branch: b2g_ics_1.2

Comment 9

[Michael Vines [:m1] [:evilmachines]]

Stay tuned, it's landed honest :)
It has yet to be pushed to CAF due to unrelated issues, might be more toward the latter half of the week before it appears (at which point that branch will become valid).

Comment 10

[Albert [:albert]]

Fixed for hamachi