[B.A.T.M.A.N.] [PATCH] alfred: Allow setting the source mac via unix sock

[B.A.T.M.A.N.] [PATCH] alfred: Allow setting the source mac via unix sock

[Dominik Heidler]

The server will only overwrite the mac if it is zero.

Signed-off-by: Dominik Heidler <dominik@heidler.eu>
---

The alfred client sets the mac to zero by default
so this shouldn't break existing behaviour.


 unix_sock.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/unix_sock.c b/unix_sock.c
index 3c7e583..12a10e6 100644
--- a/unix_sock.c
+++ b/unix_sock.c
@@ -119,7 +119,9 @@ static int unix_sock_add_data(struct globals *globals,
 
 	data = push->data;
 	data_len = ntohs(data->header.length);
-	memcpy(data->source, &interface->hwaddr, sizeof(interface->hwaddr));
+	static const char zero[ETH_ALEN] = { 0 };
+	if (!memcmp(zero, data->source, sizeof(data->source)))
+		memcpy(data->source, &interface->hwaddr, sizeof(interface->hwaddr));
 
 	if ((int)(data_len + sizeof(*data)) > len)
 		goto err;
-- 
2.7.0

Re: [B.A.T.M.A.N.] [PATCH] alfred: Allow setting the source mac via unix sock

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

On Wednesday 17 February 2016 00:51:10 Dominik Heidler wrote:
> The server will only overwrite the mac if it is zero.
> 
> Signed-off-by: Dominik Heidler <dominik@heidler.eu>

I think this breaks the forwarding/synchronization rules of alfred.

@Simon, what do you say?

kind regards,
	Sven

> ---
> 
> The alfred client sets the mac to zero by default
> so this shouldn't break existing behaviour.
> 
> 
>  unix_sock.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/unix_sock.c b/unix_sock.c
> index 3c7e583..12a10e6 100644
> --- a/unix_sock.c
> +++ b/unix_sock.c
> @@ -119,7 +119,9 @@ static int unix_sock_add_data(struct globals *globals,
>  
>  	data = push->data;
>  	data_len = ntohs(data->header.length);
> -	memcpy(data->source, &interface->hwaddr, sizeof(interface->hwaddr));
> +	static const char zero[ETH_ALEN] = { 0 };
> +	if (!memcmp(zero, data->source, sizeof(data->source)))
> +		memcpy(data->source, &interface->hwaddr, sizeof(interface->hwaddr));
>  
>  	if ((int)(data_len + sizeof(*data)) > len)
>  		goto err;
> 

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH] alfred: Allow setting the source mac via unix sock

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

On Wednesday 17 February 2016 00:51:10 Dominik Heidler wrote:
> The alfred client sets the mac to zero by default
> so this shouldn't break existing behaviour.

This also may not to be true. Some users of alfred may not set the source in 
alfred_data and thus they may end up with random bytes when reusing a buffer, 
allocating a buffer without memset or using an uninitialized buffer on stack.

It is bad practice to send uninitialized bytes around but I just wanted to 
point out that potential problem.

Your patch also doesn't explain why this feature may be required.

Btw. you can grep for "data_source" to find the code related to the 
sync/forwarded rules of alfred.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH] alfred: Allow setting the source mac via unix sock

[Dominik Heidler]

Am 17.02.2016 um 09:39 schrieb Sven Eckelmann:
> Your patch also doesn't explain why this feature may be required.

I'm running an alfred master server with this patch, so that I can run a script that will
inject data of legacy clients (that don't have an alfred slave, but need to be crawled).

Regards,
Dominik

Re: [B.A.T.M.A.N.] [PATCH] alfred: Allow setting the source mac via unix sock

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

On Wednesday 17 February 2016 11:36:01 Dominik Heidler wrote:
> Am 17.02.2016 um 09:39 schrieb Sven Eckelmann:
> > Your patch also doesn't explain why this feature may be required.
> 
> I'm running an alfred master server with this patch, so that I can run a
> script that will inject data of legacy clients (that don't have an alfred
> slave, but need to be crawled).

Sorry for the late reply. In generally I think this change is acceptable, 
however I'd like to request the following changes and to resend your patch: 

 * Please write a proper commit message in the patch, explaining your scenario 
and why you need it
 * Write a comment above the change you made to explain that clients should 
set the mac to 00:00:00:00:00:00 by default, unless they want alfred to use 
its mac for them.

Thank you!
     Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]