From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH 1/3] allow remote syslog via TCP in syslogdctrl.c
Date: Tue, 14 Nov 2017 14:37:33 +0000
Message-ID: <1510670253.4838.430.camel@ipfire.org>
In-Reply-To: <20171112081328.6a4b3621.peter.mueller@link38.eu>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============4813960320527238346=="
List-Id: <development.lists.ipfire.org>

--===============4813960320527238346==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hello,

On Sun, 2017-11-12 at 08:13 +0100, Peter M=C3=BCller wrote:
> Make syslogctrl.c use TCP as remote logging file if specified so.
>=20
> NOTE: This patch likely contains errors, since I do not know anything
> about C at all. Please have a close look at it. Sorry.

Yes it has some errors and won't work. Not sure how you tested this
successfully.

> Signed-off-by: Peter M=C3=BCller <peter.mueller(a)link38.eu>
> ---
>  src/misc-progs/syslogdctrl.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>=20
> diff --git a/src/misc-progs/syslogdctrl.c b/src/misc-progs/syslogdctrl.c
> index 52719023e..b356ebe49 100644
> --- a/src/misc-progs/syslogdctrl.c
> +++ b/src/misc-progs/syslogdctrl.c
> @@ -32,13 +32,14 @@
> =20
>  int main(void)
>  {
> -   char buffer[STRING_SIZE], command[STRING_SIZE], hostname[STRING_SIZE];
> +   char buffer[STRING_SIZE], command[STRING_SIZE], hostname[STRING_SIZE],
> protocol[STRING_SIZE];
>     char varmessages[STRING_SIZE], asynclog[STRING_SIZE];
>     int config_fd,rc,fd,pid;
>     struct stat st;
>     struct keyvalue *kv =3D NULL;
>     memset(buffer, 0, STRING_SIZE);
>     memset(hostname, 0, STRING_SIZE);
> +   memset(protocol, 0, STRING_SIZE);
>     memset(varmessages, 0, STRING_SIZE);
>     memset(asynclog, 0, STRING_SIZE);

There is no need to call memset here, but since it is for everything else it
probably doesn't hurt.

Better would be to initialise the string with "" right when it is defined. Wo=
uld
spare the memset at runtime.

Or to change findkey() to set the string to "" when the key wasn't found.

> @@ -73,6 +74,12 @@ int main(void)
>        exit(ERR_SETTINGS);
>     }
> =20
> +   if (!findkey(kv, "REMOTELOG_PROTOCOL", protocol))
> +   {
> +      fprintf(stderr, "Cannot read REMOTELOG_PROTOCOL\n");
> +      exit(ERR_SETTINGS);
> +   }
> +
>     freekeyvalues(kv);

Should we not default to UDP here in case the value is not being set?

I think exiting is a bit much.
 =20
> @@ -105,9 +112,22 @@ int main(void)
>        exit(ERR_CONFIG);
>     }
> =20
> +   /* differ between UDP and TCP as rsyslog protocol */
>     if (!strcmp(buffer,"on"))
> -      snprintf(buffer, STRING_SIZE - 1, "/bin/sed -e
> 's/^#\\?\\(\\*\\.\\*[[:blank:]]\\+@\\).\\+$/\\1%s/' /etc/syslog.conf >&%d",
> hostname, config_fd );
> +      if ( protocol =3D=3D "udp" )
> +      {
> +         snprintf(buffer, STRING_SIZE - 1, "/bin/sed -e
> 's/^#\\?\\(\\*\\.\\*[[:blank:]]\\+@\\).\\+$/\\1%s/' /etc/syslog.conf >&%d",
> hostname, config_fd );
> +      }
> +      elif ( protocol =3D=3D "tcp" )
> +      {
> +         snprintf(buffer, STRING_SIZE - 1, "/bin/sed -e
> 's/^#\\?\\(\\*\\.\\*[[:blank:]]\\+@@\\).\\+$/\\1%s/' /etc/syslog.conf >&%d",
> hostname, config_fd );
> +      }
> +      else
> +      {
> +         fprintf(stderr, "Received invalid protocol for remote log\n");
> +      }
>     else
> +      /* turn off remote syslog if specified */
>        snprintf(buffer, STRING_SIZE - 1, "/bin/sed -e
> 's/^#\\?\\(\\*\\.\\*[[:blank:]]\\+(a).\\+\\)$/#\\1/' /etc/syslog.conf >&%d",
> config_fd );
> =20
>       /* if the return code isn't 0 failsafe */

This block is slightly messy with calling all the seds. But I it has been like
this before, performance is not important here, etc.

But you can't compare strings in C like this. You will compare the pointer
values here which will never match.

The correct check would be: strcmp(protocol, "udp") =3D=3D 0

And likewise for TCP.

Best,
-Michael

--===============4813960320527238346==
Content-Type: application/pgp-signature
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename="signature.asc"
MIME-Version: 1.0

LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KCmlRSXpCQUFCQ2dBZEZpRUU1L3JXNWwzR0dl
Mnlwa3R4Z0hudy8yK1FDUWNGQWxvSy82NEFDZ2tRZ0hudy8yK1EKQ1FlR0toQUFsQnZHOEZSWlN6
d0pPWnhxOHRXRVFXdi8xbzA5OGNVUVZ6OGZjdS9oRE9UeVRTbDFQY1NqMENpQwp0U3pQR1RrUkV0
QnJxZlVta0ZLQmY4R3JpcFIxdWVuNVBSMVlRQnM1NVdUNGw1YXdhN1huMDY3a2JGS29KYjlBCkl2
a0IzajZiWHF2bFp6UUhWTjNkYzdnOEMza1VwcnRnd2ZjVWdPWVRHVm9NVkdLaUhxM2Qvc0hBOTVq
c21wWkcKNW54VGdxZUM3VkFMS1V2QmpDWnZmRFRLVFFZRmpLYm9RZW1rZysxMXpFT0ZPUVNXbDF4
TWUvZldXdTJrcHdLbwpuanJlUHFhczVYZlhiKzhnalJ5a094KzEzWHdwbzZ1SHc1VjhwRHI0bVFC
UnN4WnVUL3BIWnBhSjZMRkVDU2VvClh3R1VUdHJhTWlFZ0dpL2ZOR243OVErM0ozaFM4ZEdKSlJl
VzdyRXpSZERUYXl5TUUyYmtZNGJSTEU5SzJRTTQKWmV0djRxeFBXYjU3cnJjOXZuQmlOR2JNdVps
ZmIxQTE5bVIvcGRReUpIMjl1aUZQRERod1kvS0V0WW1zbFNjUAoxVHQyL3ZXYUdEYVllaW5veDV5
dEpCUkdTWjRBUXBOeitzY3VKRzJZc3Jndncrcjg2OTQzMjErWi9RMzJxamVsCmZWWWczaU9Ec05R
SHNudHJMWXNNb0Y1S1dBa1VPUFBxZDdKOWZXbG1hTGcyenRzQzhPNDNVd09SWlFhQ1BEWjQKdnBB
ZGhPejhtVE1aSm1SRzhnRjY5OVFEOTZpNFFGempiQk1ud1BkN2ZheGYzMVJLTFFHd3R5aU84QmpH
Vkl0cAp4d3RlRCtYUFNDRE1yZ2h0REhGOXFYOHdkR3lPTGk5SjZERlFFT3dJcEhvdWltbkg1aTA9
Cj1MZXZoCi0tLS0tRU5EIFBHUCBTSUdOQVRVUkUtLS0tLQo=

--===============4813960320527238346==--