Agreed.
I'll send a cosmetic patch in separate post.
--
Vladimir
2008/3/24, Denys Vlasenko <[EMAIL PROTECTED]>:
> On Monday 24 March 2008 19:45, [EMAIL PROTECTED] 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
>
_______________________________________________
busybox mailing list
[email protected]
http://busybox.net/cgi-bin/mailman/listinfo/busybox
