LPD: working again

[Denys Vlasenko]

On Monday 24 March 2008 19:45, dronnikov at gmail.com wrote:
> Hello!
> 
> The problem of current SVN was that we overwrite subcommand s[0] while reading positive answer from client!

Yes, indeed. But I do prefer to read that ACK _before_ anything else
is done - I don't want to do anything with the file unless
I am sure it is not corrupted.
So I moved "read ACK" code back up, and replaced s/s[0]/s[1]/

                real_len = bb_copyfd_size(STDIN_FILENO, fd, expected_len);
                if (real_len != expected_len) {
                        printf("Expected %d but got %d bytes\n",
                                expected_len, real_len);
                        goto err_exit;
                }
                // get ACK and see whether it is NUL (ok)
                if (safe_read(STDIN_FILENO, &s[1], 1) != 1 || s[1] != 0) {
                        goto err_exit;
                }

> Still I think we setup environment in exec_helper() too restrictive: xsetenv IMO should be reverted back 
> to putenv().

Well, now it's just a matter of code size.

                        var[0] = *q++;
                        xsetenv(var, q);

is smaller than

                        putenv(xasprintf("%c=%s", *q, q+1));

> Next, I find having two sources of data filename is very bad. We should use the only one (IMO DATAFILE) and rip 
> the other completely.

Yes, datafile's name is always known, it is either
in subcommand 3 (datafile) header (if clients sends "3, then 2")
or in subcommand 2 (ctrlfile) body (if client goes "2, then 3" road).

If you can shrink code by using only one - great.

> I tried to place comments at any of spots I have changed in patch.
> Denys, please consider and comment!

I removed FEATURE_LPD_PARANOIA. We _really_ cannot trust remote peer,
and must be paranoid.

1. Why we need control file size check:
        // read and delete ctrlfile
        q = xmalloc_open_read_close(filenames[0], NULL);
what will happen if remote peer sent us 1 GB controlfile, eh?
How many admins will be careful enough to run lpd under setlimit -m 999999
to prevent remote OOM attack? No sane client will ever need
to send big controlfiles, so size check is ok.

2. Duplicate subcommand:
+               if (spooling & (1 << (s[0]-1))) {
                        printf("Duplicated subcommand\n");
otherwise broken client can send us many controlfiles
and only one datafile, cluttering up spool directory.
Or vice versa...
I'd rather scream, and refuse to process such job.

3. Why isalpha(1st char of ctrlfile line): examples of bad/malicious lines:
    "\0PATH=something", "=DIE", ""


                // get ACK and see whether it is NUL (ok)

Thanks for debugging!
--
vda