[Toybox] [PATCH] Fix the operator precedence in expr.

[Rob Landley]

On 03/15/2016 04:15 PM, Andy Chu wrote:
> Here is a second patch on top of the first one.  Between these two
> patches, it's pretty much a complete rewrite -- different algorithm,
> data structures, and code structure.
> 
> It's back at 276 lines, which I think compares pretty favorably to
> busybox at 538 lines:
> 
> http://code.metager.de/source/xref/busybox/coreutils/expr.c
> 
> One thing is that I don't quite understand the memory management
> strategy.  I don't know exactly what CFG_TOYBOX_FREE is for.

Every config symbol should have menuconfig help text. If it's
not clear what it's for from that, I need to fix the help text.

config TOYBOX_FREE
        bool "Free memory unnecessarily"
        default n
        help
          When a program exits, the operating system will clean up after it
          (free memory, close files, etc). To save size, toybox usually relies
          on this behavior. If you're running toybox under a debugger or
          without a real OS (ala newlib+libgloss), enable this to make toybox
          clean up after itself.

> The code already had two issues:

Things in pending are usually in pending for more than one reason. :)

(If it was an easy fix I'd already have done it...)

> - xmprintf in re() leaks I think
> - in num_to_str, the same static buffer was used for multiple strings,
> which is a latent bug because you could strcmp() the same buffer
> against itself

That's ok if the users track the allocations, but not having done
the cleanup on it I couldn't tell you.

(I wouldn't want lib code to do that without a BIG COMMENT, but
local to a command I assume the command author knows what they're
doing. At least when it's not in pending and I've _confirmed_ they
know what they're doing, at least well enough to fool me. :)

> I changed it to use xmalloc, which will cause a memory leak too.  I
> guess if I want to clean up properly, I should keep a list of pointers
> in GLOBALS and free them all at the end in expr_main() ?

If it's at the end of expr_main() we don't care. However, if leaks
per-input and we have unlimited input... (I believe the current linux
environment space limit is INT_MAX arguments each INT_MAX long, modulo
ulimit applying something to that maybe?)

This sounds per-argument, so I'd want to free it when it's done
with it if we can. (I need to read the code after applying your
patches.)

> BTW this 0001- patch is more up to date than the one in the previous
> e-mail (I used my @google.com e-mail address because my employer wants
> that -- it's stricter now due to all the lawsuits...)

Works for me.

I'm currently squinting at 3 different FAT docs, but I'll try to
take a look at this after dinner.

> Andy

Thanks,

Rob